[PATCH] D13358: InstSimplify: Fold comparisons between allocas and arguments under certain circumstances

[Hans Wennborg via llvm-commits]

hans marked 2 inline comments as done.

================
Comment at: lib/Analysis/InstructionSimplify.cpp:2130
@@ +2129,3 @@
+    // the alloca and argument compare unequal. To do this, the alloca mustn't
+    // be observed, and we must fold *all* comparisons between pointers based
+    // on the alloca and argument consistently.
----------------
sanjoy wrote:
> I have a question about this second bit -- do we have to ensure that we only *fold* such comparisons consistently, or do we have to ensure that they also evaluate consistently?  IOW, say I have
> 
> ```
> void f(i8* %arg) {
>  i8* %val = alloca
>  %t0 = %val == %arg
>  ...
>  %t1 = complex_xform(%val) == %arg
> ```
> 
> such that `complex_xform(x) == x` always, but the compiler cannot prove that.  Consequently we fold `%t0` to `false`, but don't fold `complex_xform(x) == x` at all, and it ends up evaluating to `true` at runtime.
> 
> Are you relying on the fact that a `complex_xform` cannot be formed out of the operators you look through in `CanTrackAlluses`?  I'd be more comfortable if you folded *all* comparisons based off of `%alloca` in one go.
Yes, exactly: the comparisons have to evaluate consistently, so we have to fold them all, and I'm relying on CanTrackAllUses (maybe not a great name actually) to ensure that we can do that.

I don't think I could fold all comparisons based on an %alloca in one go from InstSimplify as it's actually not changing the instruction, just returning a new value for it. Maybe there's a better place to do this?

On the other hand, I don't think multiple comparisons is very common. Maybe we should just bail out if we see another comparison? I'll do that for now.

================
Comment at: lib/Analysis/InstructionSimplify.cpp:2137
@@ +2136,3 @@
+
+      auto CanTrackAllUses = [DL](Value *V) {
+        unsigned MaxDepth = 16;
----------------
sanjoy wrote:
> Can this be rewritten to use `PointerMayBeCaptured`?
My check is much more conservative. For example, it won't follow the use through a phi node, and certainly won't allow it in a function call - even if the function is nocapture, it could make comparisons with the pointer.

David pointed out that I should probably break this out into a separate function though, so I'll do that.

================
Comment at: lib/Analysis/InstructionSimplify.cpp:2168
@@ +2167,3 @@
+          } else if (isa<IntrinsicInst>(I)) {
+            // Intrinsics should be safe (other calls would escape the pointer).
+            continue;
----------------
sanjoy wrote:
> `patchpoint`, `stackmap` and `gc.statepoint` all can escape values -- they're basically a call to an unknown function (I don't know if they'll show up as an `IntrinsicInst` though).
Thanks. David also pointed out he had some intrinsic that would escape a pointer. I'll build a whitelist here.


http://reviews.llvm.org/D13358