[PATCH] D109760: [GVN] Simple GVN hoist

[Momchil Velikov via Phabricator via llvm-commits]

chill abandoned this revision.
chill marked 6 inline comments as done.
chill added a comment.

This abandoned in favor of https://reviews.llvm.org/D110817 and https://reviews.llvm.org/D110822



================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:2494
+    bool RunAgain = true;
+    while (RunAgain) {
+      bool HoistedAny;
----------------
junparser wrote:
> It is better to add parameter like gvn-hoist-max-chain-length in gvnhoist here based on running data from spec.
It's in the works.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:2842
+  return I.isVolatile() || I.isAtomic() ||
+         !isGuaranteedToTransferExecutionToSuccessor(&I);
+}
----------------
junparser wrote:
> 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.
We don't hoist individual instructions - only pairs. This is part of the way we make sure a we won't speculatively execute volatile/atomic instructions, nor we'd change the number and ordering of these in any execution sequence. If all other correctness conditions are satisfied, we can merge a volatile (or atomic) instruction with an instruction from the other block with the same volatile-ty and atomic ordering.  This is fixed in the new series of patches. 


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

https://reviews.llvm.org/D109760