GVH-hoist: only clone GEPs (PR28606)

[sebpop]

Summary

We should not clone stored values unless they are GEPs that are special cased to avoid hoisting them without hoisting their associated ld/st.

[sebpop]

This patch passes make check. I am currently testing this on the test-suite.

[majnemer]

llvm/lib/Transforms/Scalar/GVNHoist.cpp

586

We could put the check here:

...
GetElementPtrInst *Val = nullptr;
if (auto *St = dyn_cast<StoreInst>(Repl)) {
  Gep = ...
  Val = dyn_cast<GetElementPtrInst>(St->getValueOperand());

[sebpop]

llvm/lib/Transforms/Scalar/GVNHoist.cpp

586

I think this is not possible: we need to check whether the stored value is available at HoistPt.
If we dyn_cast Val to GEP this early we would only hoist GEP stores.

[majnemer]

llvm/lib/Transforms/Scalar/GVNHoist.cpp

586

Oh, I understand now. You are trying to move the address computation closer to the memory operation...

592–594

This can be removed now, we won't try to clone it.

[sebpop]

Also remove the check for stored PHI nodes.

[majnemer]

LGTM

[eli.friedman]

llvm/lib/Transforms/Scalar/GVNHoist.cpp

588

I think you want "Val && !DT->dominates(Val->getParent(), HoistPt)", or something like that? You care whether the value itself is available, not its operands.

[sebpop]

llvm/lib/Transforms/Scalar/GVNHoist.cpp

588

This is correct. We do not need to check all operands of the Val instruction.

[sebpop]

For everything except GEPs, only check that the stored value is available: do not check the availability of the operands.

[eli.friedman]

You're missing an allOperandsAvailable call if Val is a GEP. (I'm not sure why you're even trying to special-case GEPs here.)

[sebpop]

In D22652#492205, @eli.friedman wrote:

You're missing an allOperandsAvailable call if Val is a GEP. (I'm not sure why you're even trying to special-case GEPs here.)

GEPs are hoisted only when the ld or st can be moved.

I added that check.

[sebpop]

In D22652#492234, @sebpop wrote:

I added that check.

in https://reviews.llvm.org/rL276364

llvm/lib/Transforms/Scalar/GVNHoist.cpp

bool makeOperandsAvailable(Instruction *Repl, BasicBlock *HoistPt,
                              const SmallVecInsn &InstructionsToHoist) const {
     // Check whether the GEP of a ld/st can be synthesized at HoistPt.
     GetElementPtrInst *Gep = nullptr;
     Instruction *Val = nullptr;
     if (auto *Ld = dyn_cast<LoadInst>(Repl))
       Gep = dyn_cast<GetElementPtrInst>(Ld->getPointerOperand());
     if (auto *St = dyn_cast<StoreInst>(Repl)) {
       Gep = dyn_cast<GetElementPtrInst>(St->getPointerOperand());
       Val = dyn_cast<Instruction>(St->getValueOperand());
     }
 
     if (!Gep)
       return false;
 
     // PHIs may only be inserted at the start of a block.
     if (Val && isa<PHINode>(Val))
       return false;
 
     // Check whether we can compute the Gep at HoistPt.
     if (!allOperandsAvailable(Gep, HoistPt))
       return false;
 
     // Also check that the stored value is available.
     if (Val && !allOperandsAvailable(Val, HoistPt))
       return false;
for (const Instruction *OtherInst : InstructionsToHoist) {
       else
         OtherGep = cast<GetElementPtrInst>(
             cast<StoreInst>(OtherInst)->getPointerOperand());
       ClonedGep->intersectOptionalDataWith(OtherGep);
     }
     Repl->replaceUsesOfWith(Gep, ClonedGep);
 
     // Also copy Val.
-    if (Val) {
+    if (Val && isa<GetElementPtrInst>(Val)) {
       Instruction *ClonedVal = Val->clone();
       ClonedVal->insertBefore(HoistPt->getTerminator());
       // Conservatively discard any optimization hints, they may differ on the
       // other paths.
       ClonedVal->dropUnknownNonDebugMetadata();
       for (const Instruction *OtherInst : InstructionsToHoist) {
         const auto *OtherVal =
             cast<Instruction>(cast<StoreInst>(OtherInst)->getValueOperand());

llvm/test/Transforms/GVN/hoist-pr28606.ll

+; RUN: opt -gvn-hoist -S < %s | FileCheck %s
+
+target datalayout = "e-m:x-p:32:32-i64:64-f80:32-n8:16:32-a:0:32-S32"
+target triple = "i686-pc-windows-msvc18.0.0"
+
+%struct.S = type { i8* }
+
+declare void @f(<{ %struct.S }>* inalloca)
+
+
+; Check that we don't clone the %x alloca and insert it in the live range of
+; %argmem, which would break the inalloca contract.
+;
+; CHECK-LABEL: @test
+; CHECK: alloca i8
+; CHECK: stacksave
+; CHECK: alloca inalloca
+; CHECK-NOT: alloca i8
+; CHECK: stackrestore
+
+define void @test(i1 %b) {
+entry:
+  %x = alloca i8
+  %inalloca.save = call i8* @llvm.stacksave()
+  %argmem = alloca inalloca <{ %struct.S }>, align 4
+  %0 = getelementptr inbounds <{ %struct.S }>, <{ %struct.S }>* %argmem, i32 0, i32 0
+  br i1 %b, label %true, label %false
+
+true:
+  %p = getelementptr inbounds %struct.S, %struct.S* %0, i32 0, i32 0
+  store i8* %x, i8** %p, align 4
+  br label %exit
+
+false:
+  %p2 = getelementptr inbounds %struct.S, %struct.S* %0, i32 0, i32 0
+  store i8* %x, i8** %p2, align 4
+  br label %exit
+
+exit:
+  call void @f(<{ %struct.S }>* inalloca %argmem)
+  call void @llvm.stackrestore(i8* %inalloca.save)
+  ret void
+}
+
+declare i8* @llvm.stacksave()
+declare void @llvm.stackrestore(i8*)