[llvm] r299851 - [InstCombine] fix matching of or-of-icmps constants (PR32524)

[NAKAMURA Takumi via llvm-commits]

Seems it causes miscompilation (or revealing a bug) in AMDGPUCodeGen, in
stage 2.
Investigating.

http://bb.pgr.jp/builders/clang-3stage-x86_64-linux/builds/14928

On Tue, Apr 11, 2017 at 2:08 AM Sanjay Patel via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> Author: spatel
> Date: Mon Apr 10 11:55:57 2017
> New Revision: 299851
>
> URL: http://llvm.org/viewvc/llvm-project?rev=299851&view=rev
> Log:
> [InstCombine] fix matching of or-of-icmps constants (PR32524)
>
> Also, make the same change in and-of-icmps and remove a hack for detecting
> that case.
>
> Finally, add some FIXME comments because the code duplication here is
> awful.
>
> This should fix the remaining IR problem noted in:
> https://bugs.llvm.org/show_bug.cgi?id=32524
>
>
> Modified:
>     llvm/trunk/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
>     llvm/trunk/test/Transforms/InstCombine/or.ll
>
> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp?rev=299851&r1=299850&r2=299851&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
> (original)
> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp Mon Apr
> 10 11:55:57 2017
> @@ -807,6 +807,7 @@ Value *InstCombiner::FoldAndOfICmps(ICmp
>      }
>    }
>
> +  // FIXME: The code below is duplicated in FoldOrOfICmps.
>    // From here on, we only handle:
>    //    (icmp1 A, C1) & (icmp2 A, C2) --> something simpler.
>    if (Val != Val2)
> @@ -825,11 +826,14 @@ Value *InstCombiner::FoldAndOfICmps(ICmp
>
>    // Ensure that the larger constant is on the RHS.
>    bool ShouldSwap;
> -  if (CmpInst::isSigned(PredL) ||
> -      (ICmpInst::isEquality(PredL) && CmpInst::isSigned(PredR)))
> -    ShouldSwap = LHSC->getValue().sgt(RHSC->getValue());
> -  else
> +  if (CmpInst::isUnsigned(PredL) || CmpInst::isUnsigned(PredR)) {
> +    // We have an unsigned compare (possibly with an equality compare),
> so treat
> +    // the constants as unsigned.
>      ShouldSwap = LHSC->getValue().ugt(RHSC->getValue());
> +  } else {
> +    // Equality transforms treat the constants as signed.
> +    ShouldSwap = LHSC->getValue().sgt(RHSC->getValue());
> +  }
>
>    if (ShouldSwap) {
>      std::swap(LHS, RHS);
> @@ -877,10 +881,6 @@ Value *InstCombiner::FoldAndOfICmps(ICmp
>      case ICmpInst::ICMP_SGT: // (X != 13 & X s> 15) -> X s> 15
>        return RHS;
>      case ICmpInst::ICMP_NE:
> -      // Special case to get the ordering right when the values wrap
> around
> -      // zero.
> -      if (LHSC->getValue() == 0 && RHSC->getValue().isAllOnesValue())
> -        std::swap(LHSC, RHSC);
>        if (LHSC == SubOne(RHSC)) { // (X != 13 & X != 14) -> X-13 >u 1
>          Constant *AddC = ConstantExpr::getNeg(LHSC);
>          Value *Add = Builder->CreateAdd(Val, AddC, Val->getName() +
> ".off");
> @@ -1727,6 +1727,7 @@ Value *InstCombiner::FoldOrOfICmps(ICmpI
>          return Builder->CreateICmpULE(Val, LHSC);
>    }
>
> +  // FIXME: The code below is duplicated in FoldAndOfICmps.
>    // From here on, we only handle:
>    //    (icmp1 A, C1) | (icmp2 A, C2) --> something simpler.
>    if (Val != Val2)
> @@ -1745,11 +1746,14 @@ Value *InstCombiner::FoldOrOfICmps(ICmpI
>
>    // Ensure that the larger constant is on the RHS.
>    bool ShouldSwap;
> -  if (CmpInst::isSigned(PredL) ||
> -      (ICmpInst::isEquality(PredL) && CmpInst::isSigned(PredR)))
> -    ShouldSwap = LHSC->getValue().sgt(RHSC->getValue());
> -  else
> +  if (CmpInst::isUnsigned(PredL) || CmpInst::isUnsigned(PredR)) {
> +    // We have an unsigned compare (possibly with an equality compare),
> so treat
> +    // the constants as unsigned.
>      ShouldSwap = LHSC->getValue().ugt(RHSC->getValue());
> +  } else {
> +    // Equality transforms treat the constants as signed.
> +    ShouldSwap = LHSC->getValue().sgt(RHSC->getValue());
> +  }
>
>    if (ShouldSwap) {
>      std::swap(LHS, RHS);
>
> Modified: llvm/trunk/test/Transforms/InstCombine/or.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/or.ll?rev=299851&r1=299850&r2=299851&view=diff
>
> ==============================================================================
> --- llvm/trunk/test/Transforms/InstCombine/or.ll (original)
> +++ llvm/trunk/test/Transforms/InstCombine/or.ll Mon Apr 10 11:55:57 2017
> @@ -223,10 +223,9 @@ define i1 @test19(i32 %A) {
>
>  define i1 @or_icmps_eq_diff1(i32 %x) {
>  ; CHECK-LABEL: @or_icmps_eq_diff1(
> -; CHECK-NEXT:    [[CMP1:%.*]] = icmp eq i32 %x, -1
> -; CHECK-NEXT:    [[CMP2:%.*]] = icmp eq i32 %x, 0
> -; CHECK-NEXT:    [[LOGIC:%.*]] = or i1 [[CMP1]], [[CMP2]]
> -; CHECK-NEXT:    ret i1 [[LOGIC]]
> +; CHECK-NEXT:    [[X_OFF:%.*]] = add i32 %x, 1
> +; CHECK-NEXT:    [[TMP1:%.*]] = icmp ult i32 [[X_OFF]], 2
> +; CHECK-NEXT:    ret i1 [[TMP1]]
>  ;
>    %cmp1 = icmp eq i32 %x, -1
>    %cmp2 = icmp eq i32 %x, 0
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170411/fe37b720/attachment.html>