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

[Usman Nadeem via Phabricator via llvm-commits]

mnadeem marked 2 inline comments as done.
mnadeem added inline comments.


================
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
----------------
nikic wrote:
> 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).
> You'll want to use clang-format. https://llvm.org/docs/Contributing.html#format-patches

It is already clang-formatted, not sure why the formatting is now different. 
`git diff -U0 --relative HEAD~ | clang/tools/clang-format/clang-format-diff.py -p1 -sort-includes -i`




================
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
----------------
mnadeem wrote:
> nikic wrote:
> > 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).
> > You'll want to use clang-format. https://llvm.org/docs/Contributing.html#format-patches
> 
> It is already clang-formatted, not sure why the formatting is now different. 
> `git diff -U0 --relative HEAD~ | clang/tools/clang-format/clang-format-diff.py -p1 -sort-includes -i`
> 
> 
> 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).

I replaced `From` with `Use &`, but I still needed the To param. Can you look at loads.cpp to see if it makes sense to use both the use and the to value?


================
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.
----------------
nikic wrote:
> 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.
I created an `isPointerUseReplacable(const Use &U` but made it a static function which can be exposed in the future.
I have also taken some of the checks out of the recursive function.


================
Comment at: llvm/lib/Analysis/Loads.cpp:699
+  if (isa<PHINode>(U) && all_of(U->users(), [&](const User *User) {
+        return isa<PHINode>(User) ||
+               canReplacePointersRecursive(U, To, User, MaxLookup - 1);
----------------
aqjune wrote:
> mnadeem wrote:
> > aqjune wrote:
> > > Could you elaborate a bit about `isa<PHINode>(User)` condition?
> > > I think that in the following case replacing %p with %q should fail, but this condition seems to allow it:
> > > ```
> > > BB1:
> > >   %phi1 = phi [%p, <pred>], ..
> > >   br BB2
> > > BB2:
> > >   %phi2 = phi [%phi1, <BB1>], ..
> > >   store i32 0, ptr %phi2
> > > 
> > > ```
> > I was trying to handle something like the example below. I guess I didn't think about the cases where the second phi's use could have illegal replacements.
> > 
> > Not sure what the best way to handle this would be.
> > 
> > ```
> > BB1:
> >   %phi1 = phi [%p, <pred>], ..
> >   %cond = icmp eq ptr %phi1 , %q    <<---- This case
> >   br BB2
> > BB2:
> >   %phi2 = phi [%phi1, <BB1>], ..
> >   store i32 0, ptr %phi2
> > ```
> I think dealing with this case is tricky.
> To replace `%p` in the operand of `%phi1`, we should copy it as follows:
> ```
> BB1:
>   %phi1 = phi [%p, <pred>], ..
>   %phi1_copy = phi [%p, <pred>], .. <<-- we can replace %p with %q here
>   %cond = icmp eq ptr %phi1_copy , %q    <<---- ... because %phi1_copy's only use is this icmp.
>   br BB2
> BB2:
>   %phi2 = phi [%phi1, <BB1>], ..
>   store i32 0, ptr %phi2
> ```
I fixed the code and added a test case in a previous revision. I think fixing this will need more extensive changes than can be easily done in replaceDominatedUsesWith


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:2564
   // Remove it!
   patchAndReplaceAllUsesWith(I, Repl);
   if (MD && Repl->getType()->isPtrOrPtrVectorTy())
----------------
mnadeem wrote:
> aqjune wrote:
> > As @nikic suggested in the Discourse thread, do you want to fix `patchAndReplaceAllUsesWith` so that if it is a pointer it allows replacing uses at instructions that are unaware of provenance? I wonder how things will go.
> > Actually, we can count different assembly files rather than LLVM IR. If it is less than 10, then we can investigate the files more deeply. Actually, if there are diffs even after allowing this, I think it might be miscompilation bugs.
> Maybe in a separate patch? I tried doing assembly files but the changes were more extensive due to register assignment changes.
I have guarded `addToLeaderTable` with the check so IIUC this is not needed any more?


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

https://reviews.llvm.org/D143129