[llvm] 41e68f7 - [EarlyCSE] Fix and recommit the revised c9826829d74e637163fdb0351870b8204e62d6e6

[Roman Lebedev via llvm-commits]

On Fri, Sep 11, 2020 at 6:31 AM Michael Liao via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
>
>
> Author: Michael Liao
> Date: 2020-09-10T23:30:56-04:00
> New Revision: 41e68f7ee7b3bb33e9acb0502339a858806e8523
>
> URL: https://github.com/llvm/llvm-project/commit/41e68f7ee7b3bb33e9acb0502339a858806e8523
> DIFF: https://github.com/llvm/llvm-project/commit/41e68f7ee7b3bb33e9acb0502339a858806e8523.diff
>
> LOG: [EarlyCSE] Fix and recommit the revised c9826829d74e637163fdb0351870b8204e62d6e6
Nit: commit hashes aren't really meaningful. best to just say
"Recommit: <original subject>"

> In addition to calculate hash consistently by swapping SELECT's
> operands, we also need to inverse the select pattern favor to match the
> original logic.
>
> [EarlyCSE] Equivalent SELECTs should hash equally
>
> DenseMap<SimpleValue> assumes that, if its isEqual method returns true
> for two elements, then its getHashValue method must return the same value
> for them. This invariant is broken when one SELECT node is a min/max
> operation, and the other can be transformed into an equivalent min/max by
> inverting its predicate and swapping its operands. This patch fixes an
> assertion failure that would occur intermittently while compiling the
> following IR:
>
>     define i32 @t(i32 %i) {
>       %cmp = icmp sle i32 0, %i
>       %twin1 = select i1 %cmp, i32 %i, i32 0
>       %cmpinv = icmp sgt i32 0, %i
>       %twin2 = select i1 %cmpinv,  i32 0, i32 %i
>       %sink = add i32 %twin1, %twin2
>       ret i32 %sink
>     }
>
> Differential Revision: https://reviews.llvm.org/D86843
>
> Added:
>
>
> Modified:
>     llvm/lib/Transforms/Scalar/EarlyCSE.cpp
>     llvm/test/Transforms/EarlyCSE/commute.ll
>
> Removed:
>
>
>
> ################################################################################
> diff  --git a/llvm/lib/Transforms/Scalar/EarlyCSE.cpp b/llvm/lib/Transforms/Scalar/EarlyCSE.cpp
> index b655204d26dd..f71a2b9e003a 100644
> --- a/llvm/lib/Transforms/Scalar/EarlyCSE.cpp
> +++ b/llvm/lib/Transforms/Scalar/EarlyCSE.cpp
> @@ -191,11 +191,26 @@ static bool matchSelectWithOptionalNotCond(Value *V, Value *&Cond, Value *&A,
>      Pred = ICmpInst::getSwappedPredicate(Pred);
>    }
>
> +  // Check for inverted variants of min/max by swapping operands.
> +  bool Inversed = false;
>    switch (Pred) {
> -  case CmpInst::ICMP_UGT: Flavor = SPF_UMAX; break;
> -  case CmpInst::ICMP_ULT: Flavor = SPF_UMIN; break;
> -  case CmpInst::ICMP_SGT: Flavor = SPF_SMAX; break;
> -  case CmpInst::ICMP_SLT: Flavor = SPF_SMIN; break;
> +  case CmpInst::ICMP_ULE:
> +  case CmpInst::ICMP_UGE:
> +  case CmpInst::ICMP_SLE:
> +  case CmpInst::ICMP_SGE:
> +    Pred = CmpInst::getInversePredicate(Pred);
> +    std::swap(A, B);
> +    Inversed = true;
> +    break;
> +  default:
> +    break;
> +  }
> +
> +  switch (Pred) {
> +  case CmpInst::ICMP_UGT: Flavor = Inversed ? SPF_UMIN : SPF_UMAX; break;
> +  case CmpInst::ICMP_ULT: Flavor = Inversed ? SPF_UMAX : SPF_UMIN; break;
> +  case CmpInst::ICMP_SGT: Flavor = Inversed ? SPF_SMIN : SPF_SMAX; break;
> +  case CmpInst::ICMP_SLT: Flavor = Inversed ? SPF_SMAX : SPF_SMIN; break;
>    default: break;
>    }
>
>
> diff  --git a/llvm/test/Transforms/EarlyCSE/commute.ll b/llvm/test/Transforms/EarlyCSE/commute.ll
> index 57c5a853a12f..a172ba81c652 100644
> --- a/llvm/test/Transforms/EarlyCSE/commute.ll
> +++ b/llvm/test/Transforms/EarlyCSE/commute.ll
> @@ -684,6 +684,26 @@ define i32 @select_not_invert_pred_cond_wrong_select_op(i8 %x, i8 %y, i32 %t, i3
>    ret i32 %r
>  }
>
> +; This test is a reproducer for a bug involving inverted min/max selects
> +; hashing
> diff erently but comparing as equal.  It exhibits such a pair of
> +; values, and we run this test with -earlycse-debug-hash which would catch
> +; the disagreement and fail if it regressed.
> +; EarlyCSE should be able to detect the 2nd redundant `select` and eliminate
> +; it.
> +define i32 @inverted_max(i32 %i) {
> +; CHECK-LABEL: @inverted_max(
> +; CHECK-NEXT:    [[CMP:%.*]] = icmp sle i32 0, [[I:%.*]]
> +; CHECK-NEXT:    [[M1:%.*]] = select i1 [[CMP]], i32 [[I]], i32 0
> +; CHECK-NEXT:    [[CMPINV:%.*]] = icmp sgt i32 0, [[I:%.*]]
> +; CHECK-NEXT:    [[R:%.*]] = add i32 [[M1]], [[M1]]
> +; CHECK-NEXT:    ret i32 [[R]]
> +  %cmp = icmp sle i32 0, %i
> +  %m1 = select i1 %cmp, i32 %i, i32 0
> +  %cmpinv = icmp sgt i32 0, %i
> +  %m2 = select i1 %cmpinv, i32 0, i32 %i
> +  %r = add i32 %m1, %m2
> +  ret i32 %r
> +}
This seems to be the same original test as in the original commit.
Did it expose the additional problem that lead to the revert?
If not, more tests are missing i think.

>  ; This test is a reproducer for a bug involving inverted min/max selects
>  ; hashing
> diff erently but comparing as equal.  It exhibits such a pair of

Roman
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits