[PATCH] D140666: [InstCombine] combine intersection for inequality icmps

[chenglin.bi via Phabricator via llvm-commits]

bcl5980 added a comment.

In D140666#4088741 <https://reviews.llvm.org/D140666#4088741>, @inclyc wrote:

>> I don't think BMask_Mixed and BMask_NotMixed will happen at the same time.
>
> `LLVM.Transforms/InstCombine::sign-test-and-or.ll` has given us an example.
>
>   define i1 @test9(i32 %a) {
>   ; CHECK-LABEL: @test9(
>   ; CHECK-NEXT:    [[TMP1:%.*]] = and i32 [[A:%.*]], -1073741824
>   ; CHECK-NEXT:    [[TMP2:%.*]] = icmp eq i32 [[TMP1]], 1073741824
>   ; CHECK-NEXT:    ret i1 [[TMP2]]
>   ;
>     %1 = and i32 %a, 1073741824
>     %2 = icmp ne i32 %1, 0
>     %3 = icmp sgt i32 %a, -1
>     %or.cond = and i1 %2, %3
>     ret i1 %or.cond
>   }
>
> I'm not sure about this, but the debugger showed me `Mask = BMask_Mixed | BMask_NotMixed`. Can you reproduce this on your machine?

Yeah, I can reproduce the case. What I mean is not it will not happen for any case. B, D is pow2, C, E is 0 can trigger that. But this case I don't think it can match the condition in your code:

  const APInt ConstT = *ConstB & *ConstD;
  if (ConstT != *ConstB && ConstT != *ConstD)

So for this transform you add, it is not a problem I think. If you really concern for that. My suggestion is like this code:

  if (Mask & (BMask_Mixed | BMask_NotMixed)) {
    const APInt *OldConstC, *OldConstE;
    if (!match(C, m_APInt(OldConstC)) || !match(E, m_APInt(OldConstE)))
      return nullptr;
  
    auto FoldBMixed = [&](ICmpInst::Predicate CC, bool IsNot) -> Value * {
      CC = IsNot ? CmpInst::getInversePredicate(CC) : CC;
      const APInt ConstC = PredL != CC ? *ConstB ^ *OldConstC : *OldConstC;
      const APInt ConstE = PredR != CC ? *ConstD ^ *OldConstE : *OldConstE;
  
      if (((*ConstB & *ConstD) & (ConstC ^ ConstE)).getBoolValue())
        return IsNot ? nullptr : ConstantInt::get(LHS->getType(), !IsAnd);
  
      if (IsNot && !ConstB->isSubsetOf(*ConstD) && !ConstD->isSubsetOf(*ConstB))
        return nullptr;
  
      APInt BD, CE;
      if (IsNot) {
        BD = *ConstB & *ConstD;
        CE = ConstC & ConstE;
      } else {
        BD = *ConstB | *ConstD;
        CE = ConstC | ConstE;
      }
      Value *NewAnd = Builder.CreateAnd(A, BD);
      Value *CEVal = ConstantInt::get(A->getType(), CE);
      return Builder.CreateICmp(NewCC, CEVal, NewAnd);
    };
  
    if (Mask & BMask_Mixed)
      return FoldBMixed(NewCC, false);
    if (Mask & BMask_NotMixed) // can be else also
      return FoldBMixed(NewCC, true);
  }

Your code is also correct I think, I just hope we can reuse code as much as possible. 
But it depends on you, the code I paste here is also not perfect. It use the flag IsNot too many times that makes the code ugly and a little more cpu overhead.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:681-682
+
+    assert((~*ConstB & ConstC) == 0);
+    assert((~*ConstD & ConstE) == 0);
+
----------------
bcl5980 wrote:
> spatel wrote:
> > bcl5980 wrote:
> > > spatel wrote:
> > > > Add a string message to each assert like "Expected impossible compares to be simplified".
> > > > 
> > > > But I'm skeptical that we can actually do that. Ie, how do we guarantee that the operands of this logic instruction were already visited before we got here? If we can't guarantee that, just bail out instead.
> > > I think the assert should be OK. simplifyICmpInst will optimize the icmp to constant at very early time before the assert triggered.
> > No - this is a mistake I've made a few times in the past. :)
> > 
> > In simple examples (one basic block), you are correct - we will visit the icmp operands **before** the bitwise logic instruction. 
> > 
> > But it is possible that we are visiting this bitwise logic instruction before visiting the icmp instructions. In that case, we can see unsimplified instructions.
> > 
> > Usually, it takes a long time before the bug is discovered in the wild, and/or we fuzz our way to a test that creates just the right sequence to trigger the problem.
> > 
> > I advise that we just 'return nullptr' here.
> Thanks for the explaination. Then maybe we should add bail out for `if (Mask & BMask_Mixed)` also.
After read the code again I find in the function `getMaskedICmpType`: the mask `BMask_Mixed` and `BMask_NotMixed` already means B&C == C , D&E == E.
So I think early bail out is redundant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140666