[PATCH] D120371: [instcombine] Generalize one-compare rule from foldAllocaCmp for noalias calls

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:1035
+    bool captured(const Use *U) override {
+      NumCmps++;
+      if (NumCmps == 1)
----------------
nikic wrote:
> nikic wrote:
> > Can you please add an explicit check here that this is an icmp user? I guess you omitted the check on the premise that you have a known icmp user, so if you see a single capture it must be that icmp. That may be true, but the fact that we also have certain non-capturing icmps makes the reasoning here very non-obvious.
> Or rather, we should check that this user it the same one we're trying to fold.
> 
> And it may be worth pointing out that this critically depends on the fact that captures are per-use, not per-user, so if we do something like `alloc + x == alloc + y`, then we'll end up not folding because we have two capturing uses in the same icmp.
I'd tried to clarify the later point in the comment, but I will expand.  


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:6053
+      if (isKnownNonZero(Op0, Q.DL, /*Depth=*/0, Q.AC, Q.CxtI, Q.DT) ||
+          isKnownNonZero(Op1, Q.DL, /*Depth=*/0, Q.AC, Q.CxtI, Q.DT)) {
+        if (isNoAliasCall(UO0))
----------------
nikic wrote:
> `malloc(4) + 1 == 1` would be a hidden null check. Though from a quick look, we would not recognize `malloc(4) + 1` as non-zero.
> 
> `malloc(4) < 4` would be another possibility, making use of malloc alignment guarantees to perform a hidden null pointer check.
> 
> Not sure what to do about these, it's rather contrived...
Oh, yuck.  I'd completely missed this possibility.  

I see two variants here, and they're a bit different.  

1) If we ptrtoint the allocation (to do the comparison), then that's a capture, and we're fine. 

2) If we do inttoptr of the RHS instead, that's essentially a *guess* of the address (i.e. it's null), but the logic about heap layouts doesn't apply because we can't just decide the allocation didn't fail (since some later use might fault.)  

I'm going to add some test cases of this variety, then restrict the transform to the non-null allocation case OR when directly comparing a non-offset allocation.  

Once that lands, I'll revisit and see if I can do something stronger.  


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

https://reviews.llvm.org/D120371