[PATCH] D37460: [GVN] Prevent LoadPRE from hoisting through guards

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added inline comments.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:2042
+    auto *II = dyn_cast<IntrinsicInst>(&I);
+    return II && II->getIntrinsicID() == Intrinsic::experimental_guard;
+  };
----------------
efriedma wrote:
> Why does this specifically apply to guards, as opposed to anything where isGuaranteedToTransferExecutionToSuccessor() is false?
Yeah, it looks like GVN presumes that if it's a straight line non-infinite loop, they have the same "guaranteed to execute" properties.

As you correctly point out, since LLVM has implicit control flow, this is not always true.
I suspect GVN's PRE is quite broken here in that respect in theory.

However, most of the effect of isGuaranteed is on memory instructions, and those almost certainly get corrected by memdep (IE it ends up respecting the same things that isGuaranteed* is respecting).
Going through the rest:

```
 if (const auto *CRI = dyn_cast<CleanupReturnInst>(I))
    return !CRI->unwindsToCaller();
  if (const auto *CatchSwitch = dyn_cast<CatchSwitchInst>(I))
    return !CatchSwitch->unwindsToCaller();
  if (isa<ResumeInst>(I))
    return false;
  if (isa<ReturnInst>(I))
    return false;
  if (isa<UnreachableInst>(I))
    return false;

```
None of these will have successors, so there shouldn't be a predecessor we hit to check anyway.
You can hoist above them, just not sink below them.

So those should be fine.

The callsite checking, except for throwing, is probably also handled by memdep's insertion of memory dependencies.

So I suspect you may be able to rig up a testcase where memdep and isGuaranteed* end up disagreeing in a way that causes this to break.  But i also suspect it's hard.

Thoughts?




https://reviews.llvm.org/D37460