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

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune added a comment.

Regarding the updated experimental results, I think people might have thoughts about them. I will leave a comment there.



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


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


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

https://reviews.llvm.org/D143129