[PATCH] D13358: InstCombine: Fold comparisons between unguessable allocas and other pointers

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

In http://reviews.llvm.org/D13358#259819, @hans wrote:

> > Right now GetUnderlyingObject does not look through PHI nodes, but if it starts to at some point, this change will fold %cmp [...]
>
>
> That would be correct, though. Or are you saying there's a problem there?


There's a //potential// problem here in justifying the transform -- in
the loop, the programmer is no longer doing a "one-off guess" anymore,
but systematically searching for the offset of a passed in argument
(which points to some slot in the caller's stack, say) from the
`alloca`.  For instance, such a loop can allow you to implement
pointer subtraction without `ptrtoint` -- `while (&ptra[diff++] != ptrb)`.

In other words, I understand the justification for the current change
to be that: an `alloca` can be allocated to any stack slot in the
current frame, so the compiler is allowed to fold any equality
comparisons to `false`.  This seems fine.  But a loop with an
incrementing induction variable can use an equality check to
"implement" an inequality check like `ult` or `slt`, and by folding
the `!=` to `true` in `while (&ptra[diff++] != ptrb)`, you're really
semantically folding `ptra < ptrb` to `false`.  I personally think
this is not a problem, but I hope I was able to communicate what the
difference is.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:786
@@ +785,3 @@
+        return nullptr; // Found more than one cmp.
+      continue;
+    } else if (auto *Intrin = dyn_cast<IntrinsicInst>(V)) {
----------------
Ah, ok.  (This is completely optional) I'd be tempted to get rid of `NumCmps` and write the check as

```
if (U != AllocaUse)
  return nullptr;
```

where `AllocaUse` is the `Use *` of the `alloca` in `ICI` which you either pass in or compute.


http://reviews.llvm.org/D13358