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

[Sanjay Patel via Phabricator via llvm-commits]

spatel updated this revision to Diff 229133.
spatel marked an inline comment as done.
spatel added a comment.

Patch updated:

1. Reduce code by using min/max helper (rGe9bf7a60a036 <https://reviews.llvm.org/rGe9bf7a60a03640e46642509947ec0b479efb8e88>).
2. I also tried to make the test clearer (rG142cbe73e9fe <https://reviews.llvm.org/rG142cbe73e9fe834e6abaf2d709b4a429ca3a9c44>).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70148/new/

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
@@ -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
@@ -6690,8 +6690,21 @@
           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.
+      if (ReductionData.isMinMax() && isa<SelectInst>(VectorizedTree)) {
+        assert(isa<SelectInst>(ReductionRoot) &&
+               "Expected min/max reduction to have select root instruction");
+
+        Value *ScalarCond = cast<SelectInst>(ReductionRoot)->getCondition();
+        Value *VectorCond = cast<SelectInst>(VectorizedTree)->getCondition();
+        ScalarCond->replaceAllUsesWith(VectorCond);
+      }
       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.229133.patch
Type: text/x-patch
Size: 2883 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20191113/27bf0784/attachment.bin>