[PATCH] D109760: [GVN] Simple GVN hoist

[Momchil Velikov via Phabricator via llvm-commits]

chill marked 3 inline comments as done.
chill added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:2842
+  return I.isVolatile() || I.isAtomic() ||
+         !isGuaranteedToTransferExecutionToSuccessor(&I);
+}
----------------
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.



================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:2860
+    // appearing as operands in hoisted instructions).
+    if (isa<GetElementPtrInst>(&I))
+      continue;
----------------
junparser wrote:
> Is there any reason not to handle GEPs?
That's to avoid hoisting GEPs, but leaving their users behind. I was following the comment from `GVNHoist.cpp`


> // Hoisting may affect the performance in some cases. To mitigate that, hoisting
> // is disabled in the following cases.
> // 1. Scalars across calls.
> // 2. geps when corresponding load/store cannot be hoisted.

I sort of assume someone measured and found that useful :D


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


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

https://reviews.llvm.org/D109760