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

[Sanjoy Das via llvm-commits]

sanjoy added a subscriber: sanjoy.

================
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.
----------------
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.

================
Comment at: lib/Analysis/InstructionSimplify.cpp:2137
@@ +2136,3 @@
+
+      auto CanTrackAllUses = [DL](Value *V) {
+        unsigned MaxDepth = 16;
----------------
Can this be rewritten to use `PointerMayBeCaptured`?

================
Comment at: lib/Analysis/InstructionSimplify.cpp:2138
@@ +2137,3 @@
+      auto CanTrackAllUses = [DL](Value *V) {
+        unsigned MaxDepth = 16;
+        SmallVector<Use *, 8> Worklist;
----------------
Minor: I'd call this `MaxIter`.

================
Comment at: lib/Analysis/InstructionSimplify.cpp:2163
@@ +2162,3 @@
+                DL.getIntPtrType(U->get()->getType())->getScalarType();
+            APInt Offset = APInt::getNullValue(IntPtrTy->getIntegerBitWidth());
+            if (!GEP->accumulateConstantOffset(DL, Offset))
----------------
Why not just use `APInt`'s constructor?

================
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;
----------------
`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).


http://reviews.llvm.org/D13358