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

[Eric Christopher via llvm-commits]

Hi Sanjay,

I'm running into a crash on valid with this patch applied:

// clang -c -O2 -msse4 repro.cc

using a = void (*)(const void *, long, int *, int *);
int b(int);
template <int, typename> void c(const void *, long, int *d, int *) {
  int a[8];
  int e[1];
  int f;
  for (int g = 0; g < 8; g += 2) {
    for (int h = 0; h < 5; ++h)
      a[3] += e;
    for (int h = 0; h < 3; ++h)
      f = b(f) * 2;
  }
  int i = *d = 0;
  for (int g = 0; g < 8; ++g)
    if (a[g] > i) {
      i = a[g];
      *d = g;
    }
}
a j;
void k() { j = c<8, char>; }

I'm going to go ahead and temporarily revert this and we can go from there :)

Thanks and sorry for any inconvenience.

-eric

On Wed, Nov 13, 2019 at 1:05 PM Sanjay Patel via Phabricator via
llvm-commits <llvm-commits at lists.llvm.org> wrote:
>
> This revision was automatically updated to reflect the committed changes.
> Closed by commit rGa3e61946c5bd: [SLP] fix miscompile on min/max reductions with extra uses (PR43948) (authored by spatel).
>
> Changed prior to commit:
>   https://reviews.llvm.org/D70148?vs=229133&id=229167#toc
>
> Repository:
>   rG LLVM Github Monorepo
>
> 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 original 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);
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits