[DAGCombiner] Mark vectors as not AllAddOne/AllSubOne on type mismatch

[patrick-rivos]

Fixes #92193.

[llvmbot]

@llvm/pr-subscribers-llvm-selectiondag

Author: Patrick O'Neill (patrick-rivos)

Changes

Fixes #92193.

---

Full diff: https://github.com/llvm/llvm-project/pull/92195.diff

2 Files Affected:

• (modified) llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (+10-2)
• (added) llvm/test/CodeGen/RISCV/dag-combine-vselect-datatype.ll (+42)

diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index a044b6dc4838a..53519274f9fa6 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -12140,10 +12140,16 @@ SDValue DAGCombiner::foldVSelectOfConstants(SDNode *N) {
   for (unsigned i = 0; i != Elts; ++i) {
     SDValue N1Elt = N1.getOperand(i);
     SDValue N2Elt = N2.getOperand(i);
-    if (N1Elt.isUndef() || N2Elt.isUndef())
+    if (N1Elt.isUndef() || N2Elt.isUndef()) {
+      AllAddOne = false;
+      AllSubOne = false;
       continue;
-    if (N1Elt.getValueType() != N2Elt.getValueType())
+    }
+    if (N1Elt.getValueType() != N2Elt.getValueType()) {
+      AllAddOne = false;
+      AllSubOne = false;
       continue;
+    }
 
     const APInt &C1 = N1Elt->getAsAPIntVal();
     const APInt &C2 = N2Elt->getAsAPIntVal();
@@ -12152,6 +12158,8 @@ SDValue DAGCombiner::foldVSelectOfConstants(SDNode *N) {
     if (C1 != C2 - 1)
       AllSubOne = false;
   }
+  assert(!(AllAddOne && AllSubOne) &&
+         "Y=X+1 and Y=X-1 cannot be true for any given X and Y.");
 
   // Further simplifications for the extra-special cases where the constants are
   // all 0 or all -1 should be implemented as folds of these patterns.
diff --git a/llvm/test/CodeGen/RISCV/dag-combine-vselect-datatype.ll b/llvm/test/CodeGen/RISCV/dag-combine-vselect-datatype.ll
new file mode 100644
index 0000000000000..3a1dbd543546a
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/dag-combine-vselect-datatype.ll
@@ -0,0 +1,42 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 4
+; RUN: llc -O1 < %s | FileCheck %s
+
+; Dag-combine used to improperly combine a vector vselect of 0 and 5 into
+; 5 + condition(0/1) because one of the two args was transformed from an i32->i64.
+
+target datalayout = "e-m:e-p:64:64-i64:64-i128:128-n32:64-S128"
+target triple = "riscv64-unknown-linux-gnu"
+
+@g.var.0 = global i8 5
+@g.arr.0 = global i32 0
+
+define i8 @foo() {
+; CHECK-LABEL: foo:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    lui a0, %hi(g.arr.0)
+; CHECK-NEXT:    li a1, 4
+; CHECK-NEXT:    sw a1, %lo(g.arr.0)(a0)
+; CHECK-NEXT:    li a0, 0
+; CHECK-NEXT:    ret
+entry:
+  store i32 4, ptr @g.arr.0, align 32
+
+  %g.var.0.val = load i8, ptr @g.var.0, align 1
+  %loaded.arr = insertelement <4 x i8> <i8 1, i8 1, i8 1, i8 1>, i8 %g.var.0.val, i64 0
+
+  %g.arr.elem.0 = load i32, ptr @g.arr.0, align 32
+  %insert.0 = insertelement <4 x i32> zeroinitializer, i32 %g.arr.elem.0, i64 0
+  %cmp.0 = icmp ult <4 x i32> %insert.0, <i32 1, i32 1, i32 1, i32 1>
+
+  %all.g.arr.elem.0 = shufflevector <4 x i32> %insert.0, <4 x i32> zeroinitializer, <4 x i32> zeroinitializer
+  %or.0 = or <4 x i32> %all.g.arr.elem.0, <i32 1, i32 1, i32 1, i32 1>
+
+  %sel.0 = select <4 x i1> %cmp.0, <4 x i32> zeroinitializer, <4 x i32> %or.0
+
+  %trunc.0 = trunc <4 x i32> %sel.0 to <4 x i8>
+
+  %mul.0 = mul <4 x i8> %loaded.arr, %trunc.0
+  %reduced.mul.0 = call i8 @llvm.vector.reduce.mul.v4i8(<4 x i8> %mul.0)
+
+  ret i8 %reduced.mul.0
+}

[patrick-rivos]

Triage/Analysis can be found in the issue: #92193

[topperc]

This loop may have been trying to ignore undefs.

[patrick-rivos]

Updated. This invalidates the assertion I added (since a vector of undefs can be both AllAddOne and AllSubOne) so I removed that as well.

[topperc]

Is the reduce needed? Can we hit the error by returning %sel.0?

[patrick-rivos]

We don't seem to hit the error when returning %sel.0 (comments generated by llc with this PR applied):
https://godbolt.org/z/z7exjrYsc

[topperc]

Thanks for checking

[topperc]

LGTM

[patrick-rivos]

Thanks for the quick reviews!
I don't have write permission - could you land this for me?