[PATCH] D70148: [SLP] fix miscompile on min/max reductions with extra uses (PR43948)

[Sanjay Patel via Phabricator via llvm-commits]

spatel created this revision.
spatel added reviewers: ABataev, RKSimon, dtemirbulatov.
Herald added subscribers: hiraditya, mcrosier.
Herald added a project: LLVM.

I noticed that we could miscompile reductions while working on the recent 2-way enhancements.

The problem appears to be limited to cases where a min/max reduction has extra uses of the compare operand to the select.

I assume that the existing test in used-reduced-op.ll also shows a miscompile, but nobody noticed in such a large test?


https://reviews.llvm.org/D70148

Files:
  llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
  llvm/test/Transforms/SLPVectorizer/X86/reduction.ll
  llvm/test/Transforms/SLPVectorizer/X86/used-reduced-op.ll


Index: llvm/test/Transforms/SLPVectorizer/X86/used-reduced-op.ll
===================================================================
--- llvm/test/Transforms/SLPVectorizer/X86/used-reduced-op.ll
+++ llvm/test/Transforms/SLPVectorizer/X86/used-reduced-op.ll
@@ -71,7 +71,7 @@
 ; CHECK-NEXT:    [[NEG_1_1:%.*]] = sub nsw i32 0, [[SUB_1_1]]
 ; CHECK-NEXT:    [[TMP46:%.*]] = select i1 [[TMP45]], i32 [[NEG_1_1]], i32 [[SUB_1_1]]
 ; CHECK-NEXT:    [[CMP12_1_1:%.*]] = icmp slt i32 [[TMP46]], [[OP_EXTRA]]
-; CHECK-NEXT:    [[NARROW:%.*]] = or i1 [[CMP12_1_1]], undef
+; CHECK-NEXT:    [[NARROW:%.*]] = or i1 [[CMP12_1_1]], [[TMP44]]
 ; CHECK-NEXT:    [[SPEC_SELECT8_1_1:%.*]] = select i1 [[CMP12_1_1]], i32 [[TMP46]], i32 [[OP_EXTRA]]
 ; CHECK-NEXT:    [[SUB_2_1:%.*]] = sub i32 [[TMP30]], [[TMP3]]
 ; CHECK-NEXT:    [[TMP47:%.*]] = icmp slt i32 [[SUB_2_1]], 0
Index: llvm/test/Transforms/SLPVectorizer/X86/reduction.ll
===================================================================
--- llvm/test/Transforms/SLPVectorizer/X86/reduction.ll
+++ llvm/test/Transforms/SLPVectorizer/X86/reduction.ll
@@ -68,7 +68,7 @@
   ret i32 %sum.0.lcssa
 }
 
-; FIXME: PR43948 - https://bugs.llvm.org/show_bug.cgi?id=43948
+; PR43948 - https://bugs.llvm.org/show_bug.cgi?id=43948
 ; The extra use of a non-vectorized element of a reduction must not be killed.
 
 define i32 @horiz_max_multiple_uses([32 x i32]* %x, i32* %p) {
@@ -91,7 +91,7 @@
 ; CHECK-NEXT:    [[TMP5:%.*]] = select i1 [[TMP4]], i32 [[TMP3]], i32 [[T4]]
 ; CHECK-NEXT:    [[C012345:%.*]] = icmp sgt i32 [[TMP5]], [[T5]]
 ; CHECK-NEXT:    [[T17:%.*]] = select i1 [[C012345]], i32 [[TMP5]], i32 [[T5]]
-; CHECK-NEXT:    [[THREE_OR_FOUR:%.*]] = select i1 undef, i32 3, i32 4
+; CHECK-NEXT:    [[THREE_OR_FOUR:%.*]] = select i1 [[TMP4]], i32 3, i32 4
 ; CHECK-NEXT:    store i32 [[THREE_OR_FOUR]], i32* [[P:%.*]], align 8
 ; CHECK-NEXT:    ret i32 [[T17]]
 ;
Index: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
===================================================================
--- llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
+++ llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
@@ -6736,8 +6736,23 @@
           VectorizedTree = VectReductionData.createOp(Builder, "op.extra", I);
         }
       }
-      // Update users.
+
+      // Update users. For a min/max reduction that ends with a compare and
+      // select, we also have to RAUW for the compare instruction feeding the
+      // reduction root. That's because the compare may have extra uses besides
+      // the final select of the reduction.
+      Instruction *RdxCmp, *Cmp;
+      if (match(cast<Instruction>(ReductionRoot),
+                m_Select(m_Instruction(RdxCmp), m_Value(), m_Value())) &&
+          (RdxCmp->getOpcode() == Instruction::ICmp ||
+           RdxCmp->getOpcode() == Instruction::FCmp) &&
+          match(VectorizedTree,
+                m_Select(m_Instruction(Cmp), m_Value(), m_Value())) &&
+          Cmp->getOpcode() == RdxCmp->getOpcode()) {
+        RdxCmp->replaceAllUsesWith(Cmp);
+      }
       ReductionRoot->replaceAllUsesWith(VectorizedTree);
+
       // Mark all scalar reduction ops for deletion, they are replaced by the
       // vector reductions.
       V.eraseInstructions(IgnoreList);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D70148.228965.patch
Type: text/x-patch
Size: 3256 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20191112/9971c633/attachment.bin>