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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140666