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

Yingchi Long via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sun Feb 5 17:38:17 PST 2023


inclyc added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:681-682
+
+    assert((~*ConstB & ConstC) == 0);
+    assert((~*ConstD & ConstE) == 0);
+
----------------
bcl5980 wrote:
> 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.
> 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.

This looks correct, and we've removed the relevant checks in the code. @spatel WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140666



More information about the llvm-commits mailing list