[PATCH] D109760: [GVN] Simple GVN hoist

[JunMa via Phabricator via llvm-commits]

junparser added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:2842
+  return I.isVolatile() || I.isAtomic() ||
+         !isGuaranteedToTransferExecutionToSuccessor(&I);
+}
----------------
chill wrote:
> junparser wrote:
> > can you give an example that when I can not transfer execution, how do subsequent instructions in BB to be hoisted?  IMHO, collectHoistCandidates should stop here.
> For example:
> 
> ```
> define dso_local i32 @f2(i32 %c, i32 %a, i32 %b) {
> entry:
>   %tobool = icmp ne i32 %c, 0
>   br i1 %tobool, label %if.then, label %if.else
> 
> if.then:
>   call void @h()
>   %0 = sdiv i32 %a, %b
>   br label %if.end
> 
> if.else:
>   call void @h()
>   %1 = sdiv i32 %a, %b
>   br label %if.end
> 
> if.end:
>   %2  = phi i32 [%0, %if.then], [%1, %if.else]
>   ret i32 %2
> }
> 
> declare dso_local void @h() readnone
> ```
> 
> `%0` and `%1` will get the same value number, but should not be hoisted, unless ` call void @h()` is also hoisted.
> We would indeed stop the iteration in `collectHoistCandidates`, but hoist the call itself. Then on the next invocation of
> `performHoist` we will have
> 
> ```
> define dso_local i32 @f2(i32 %c, i32 %a, i32 %b) {
> entry:
>   %tobool = icmp ne i32 %c, 0
>   call void @h()
>   br i1 %tobool, label %if.then, label %if.else
> 
> if.then:
>   %0 = sdiv i32 %a, %b
>   br label %if.end
> 
> if.else:
>   %1 = sdiv i32 %a, %b
>   br label %if.end
> 
> if.end:
>   %2  = phi i32 [%0, %if.then], [%1, %if.else]
>   ret i32 %2
> }
> 
> declare dso_local void @h() readnone
> ```
> 
> and **then **we'll move the `sdiv` too.
> 
The thing is can we hoist instruction when isGuaranteedToTransferExecutionToSuccessor is true or isVolatile or maythrow. I'm not sure about this. I believe we should not hoist them.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:3034
+        // Hoisting these creates opportunities for more hoisting.
+        if (P.first->mayReadOrWriteMemory() || isHoistBarrier(*P.first))
+          RunAgain = true;
----------------
chill wrote:
> junparser wrote:
> > ditto
> So, an instruction that reads or writes memory could be a local dependency. If you hoist such an instruction, it may turn the local dependency into a non-local one, thus allowing more hoisting.
> 
> I guess there are alternative approaches (see discussion on the ML) when we explicitly have a dependency instruction, e.g. a non-volatile load
> followed by a non-volatile store, aliasing. Store can be hoisted only if the load is hoisted too.
> 
> Not clear what to do when we don't have an explicit dependency, e.g. 
> 
> ```
> define dso_local i32 @f0(i32 %c, i32* %p, i32* %q) {
> entry:
>   %tobool = icmp ne i32 %c, 0
>   br i1 %tobool, label %if.then, label %if.else
> 
> if.then:
>   store volatile i32 0, i32* %p
>   store volatile i32 0, i32* %q
>   br label %if.end
> 
> if.else:
>   store volatile i32 0, i32* %p
>   store volatile i32 0, i32* %q
>   br label %if.end
> 
> if.end:
>   ret i32 0
> }
> ```
> 
> (Current patch does not handle this case as well, I guess because of MemDep caching).
same issue with volatiled load/store.


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

https://reviews.llvm.org/D109760