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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:2042
+    auto *II = dyn_cast<IntrinsicInst>(&I);
+    return II && II->getIntrinsicID() == Intrinsic::experimental_guard;
+  };
----------------
dberlin wrote:
> 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?
> 
> 
Hi Eli,

I tried to use use `isGuaranteedToTransferExecutionToSuccessor`, and it works for my example, but another tests starts failing. The test is `test7` from `Transforms/GVN/PRE/volatile.ll` which is:

  ; Does cross block PRE work with volatiles?
  define i32 @test7(i1 %c, i32* noalias nocapture %p, i32* noalias nocapture %q) {
  ; CHECK-LABEL: test7
  ; CHECK-LABEL: entry.header_crit_edge:
  ; CHECK:       %y.pre = load i32, i32* %p
  ; CHECK-LABEL: skip:
  ; CHECK:       %y1 = load i32, i32* %p
  ; CHECK-LABEL: header:
  ; CHECK:      %y = phi i32
  ; CHECK-NEXT: %x = load volatile i32, i32* %q
  ; CHECK-NEXT: %add = sub i32 %y, %x
  entry:
    br i1 %c, label %header, label %skip
  skip:
    %y1 = load i32, i32* %p
    call void @use(i32 %y1)
    br label %header
  header:
    %x = load volatile i32, i32* %q
    %y = load i32, i32* %p
    %add = sub i32 %y, %x
    %cnd = icmp eq i32 %add, 0
    br i1 %cnd, label %exit, label %header
  exit:
    ret i32 %add
  }

It now fails because this function returns false for olatile loads and stores because `they are allowed to trap`. This logic was added in patch https://reviews.llvm.org/D21167 and Sanjoy's comment about why we do that remained unanswered. I don't clearly understand why you've added that, could you please clarify why it was done and is it OK if this test now fails?

Thanks,
Max



https://reviews.llvm.org/D37460