[GVNHoist] Don't clone allocas (PR28606)

[hans]

Summary

This fixes the tests in 32-bit Windows self-hosts (see bug).

I'm anxious to get this in as the bug is blocking the 3.9 release candidate and Windows snapshot.

[majnemer]

lib/Transforms/Scalar/GVNHoist.cpp

609–614

This should logic should go earlier, before we cloned the gep.

[hans]

lib/Transforms/Scalar/GVNHoist.cpp

609–614

Oops, thanks!

[hans]

Address David's comment.

[majnemer]

This value cloning is still quite dangerous...

Consider:

define void @test(i1 %b, i8** %y, i8** %z) {
entry:
  %x = load volatile i8*, i8** %z
  br i1 %b, label %true, label %false

true:
  %p = getelementptr inbounds i8*, i8** %y, i32 0
  store i8* %x, i8** %p, align 4
  br label %exit

false:
  %p2 = getelementptr inbounds i8*, i8** %y, i32 0
  store i8* %x, i8** %p2, align 4
  br label %exit

exit:
  ret void
}

gvn-hoist will make this:

define void @test(i1 %b, i8** %y, i8** %z) {
entry:
  %x = load volatile i8*, i8** %z, align 4
  %0 = load volatile i8*, i8** %z, align 4
  store i8* %0, i8** %y, align 4
  br i1 %b, label %true, label %false

true:                                             ; preds = %entry
  br label %exit

false:                                            ; preds = %entry
  br label %exit

exit:                                             ; preds = %false, %true
  ret void
}

This is bad, we just cloned a volatile instruction.

To be honest, I'm not entirely sure why we must clone the value because we've already checked to see that it is available at the hoist point...

[sebpop]

LGTM.
Thanks for beating me at fixing it ;-)

[sebpop]

To be honest, I'm not entirely sure why we must clone the value because we've already checked to see that it is available at the hoist point

Right, I think we do not need to clone the value.
I think we still need to clone a GEP as they are not hoisted without their ld/st.
As we are after the check for allOperandsAvailable on the stored value,
we should remove all the code cloning the stored value.

[hans]

In D22644#491917, @sebpop wrote:

To be honest, I'm not entirely sure why we must clone the value because we've already checked to see that it is available at the hoist point

Right, I think we do not need to clone the value.
I think we still need to clone a GEP as they are not hoisted without their ld/st.
As we are after the check for allOperandsAvailable on the stored value,
we should remove all the code cloning the stored value.

I tried to just do that, but the hoistStores test fails:

Instruction does not dominate all uses!
  %incdec.ptr = getelementptr inbounds i16, i16* %1, i32 1
  store i16* %incdec.ptr, i16** %2, align 8

Sebastian: can you give this a shot? I probably won't get any further today.

[sebpop]

In D22644#491959, @hans wrote:

Sebastian: can you give this a shot? I probably won't get any further today.

Yes. Thanks also for the reduced testcase.

[sebpop]

As I was not able to update the diff, I created a new ticket:
https://reviews.llvm.org/D22652

[hans]

Thanks! Abandoning this one.

lib/Transforms/Scalar/GVNHoist.cpp

if (Val && !allOperandsAvailable(Val, HoistPt))
       return false;
 
     // Copy the gep before moving the ld/st.
     Instruction *ClonedGep = Gep->clone();
     ClonedGep->insertBefore(HoistPt->getTerminator());
     Repl->replaceUsesOfWith(Gep, ClonedGep);
 
     // Also copy Val.
     if (Val) {
+      // Don't copy allocas: it can either pessimize the program by turning a
+      // static alloca into a dynamic one, or worse it could break the program
+      // if it's inserted in the live range of an inalloca.
+      if (isa<AllocaInst>(Val))
+        return false;
+
       Instruction *ClonedVal = Val->clone();
       ClonedVal->insertBefore(HoistPt->getTerminator());
       Repl->replaceUsesOfWith(Val, ClonedVal);
     }
 
     return true;
   }
 

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*)