[PATCH] D143129: [GVN] Restrict equality propagation for pointers

[Nikita Popov via Phabricator via llvm-commits]

nikic requested changes to this revision.
nikic added a comment.
This revision now requires changes to proceed.

Okay, the test diffs look much better now. Some interesting remaining cases:

- `10.ll`: This is another jump threading failure. The problem here is that the phi nodes are used in `icmp`s, but not *only* in icmps, so we can't simply replace the operand there. I'm not sure what to do about this one. We could obviously duplicate the phi node and replace its non-provenance uses, but that's not going to be profitable in general. Possibly there is some specialized jump threading support one could add for this, but this is technically tricky, especially as threading can't use the dominator tree. LVI currently only tracks equality to constants.
- `101.ll` (sink) and `103.ll` (hoist): Here we have something like `if (p == q) { store v, p } else { store v, q }` where previously `store v, q` could be sunk/hoisted out of it. This isn't legal anymore. This is probably unrecoverable at the IR level, but maybe the machine layer handles it (I didn't check).

Overall I'd be willing to try landing this and seeing whether any significant performance issues arise.



================
Comment at: llvm/include/llvm/Transforms/Utils/Local.h:416
+    function_ref<bool(const Value *From, const Value *To, const User *U)>
+        ShouldReplace);
+/// Replace each use of 'From' with 'To' if that use is dominated by
----------------
aqjune wrote:
> You'll want to use clang-format. https://llvm.org/docs/Contributing.html#format-patches
The callback doesn't need From and To -- if it needs them, it can capture them from context. Instead of `User *` we should pass `Use &`, which is more general. (E.g. if some operands are provenance-dependent, but others aren't -- I have future ptr_provenance support in mind here).


================
Comment at: llvm/lib/Analysis/Loads.cpp:706
+bool llvm::canReplacePointersIfEqual(const Value *From, const Value *To,
+                                     const User *U) {
+  // Not a pointer, just return true.
----------------
I think we should split this up into two APIs: One should stay as previous any just answer the question "can this always be replaced?" and the other should only take a Use and answer the question "is this provenance-independent?" We don't need to perform some of these checks for every single use, and some of the things (e.g. addition to leader table) can only be done if all uses can be replaced.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:2325
     if (RootDominatesEnd && !isa<Instruction>(RHS))
       addToLeaderTable(LVN, RHS, Root.getEnd());
 
----------------
I don't think we can do this unless it's legal to unconditionally replace the pointers.


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

https://reviews.llvm.org/D143129