GlobalOpt: Don't optimize dllimport for initializers

[majnemer]

Summary

Referencing a dllimport variable requires actually instructions, not
just a relocation. This partially fixes PR19955.

[majnemer]

• Move this check to a more appropriate place.

[rnk]

lib/Transforms/IPO/GlobalOpt.cpp

1987–1988

IMO getNumOperands() == 0 is the common case (ints), so I'd put that check first.

test/Transforms/GlobalOpt/constantfold-initializers.ll

55–62

Why are these all called dllexport-stuff? It seems like dllimport would be the better naming.

Can you add a test to verify that globalopt fires on dllexport data? Based on the comment you fixed, it seems like it didn't used to.

56

IMO a positive CHECK line for "@dllexport = global i32* null" would make a better test.

[majnemer]

• Address review comments.

[majnemer]

lib/Transforms/IPO/GlobalOpt.cpp

1987–1988

Can't. GlobalVariable with no initializer will get caught by that check.

test/Transforms/GlobalOpt/constantfold-initializers.ll

55–62

Done.

56

Done.

[nicholas]

David Majnemer wrote:

Hi rnk, nicholas,

Referencing a dllimport variable requires actually instructions, not
just a relocation. This partially fixes PR19955.

I am confused. Your description suggests that we needs to stop
optimizing the dllimport case. Your patch adds to the set of cases where
we perform the optimization. You also removed dllexport from the comment
listing cases we can't optimize. What is going on?

http://reviews.llvm.org/D4249

Files:

lib/Transforms/IPO/GlobalOpt.cpp
test/Transforms/GlobalOpt/constantfold-initializers.ll

Index: lib/Transforms/IPO/GlobalOpt.cpp

• lib/Transforms/IPO/GlobalOpt.cpp

+++ lib/Transforms/IPO/GlobalOpt.cpp
@@ -2054,9 +2054,8 @@

  return false;

if (GlobalVariable *GV = dyn_cast<GlobalVariable>(C))

• // Do not allow weak/*_odr/linkonce/dllimport/dllexport linkage or
• // external globals.
• return GV->hasUniqueInitializer();

+ // Do not allow weak/*_odr/linkonce linkage, dllimport, or external globals.
+ return GV->hasUniqueInitializer() || GV->hasDLLImportStorageClass();

if (ConstantExpr *CE = dyn_cast<ConstantExpr>(C)) {
  // Handle a constantexpr gep.

Index: test/Transforms/GlobalOpt/constantfold-initializers.ll

• test/Transforms/GlobalOpt/constantfold-initializers.ll

+++ test/Transforms/GlobalOpt/constantfold-initializers.ll
@@ -50,7 +50,19 @@

  ret void
}

+; PR19955
+
+@dllexportptr = global i32* null, align 4
+; CHECK-NOT: @dllexportptr = global i32* @dllexportvar, align 4
+@dllexportvar = external dllimport global i32
+define internal void @test3() {
+entry:
+ store i32* @dllexportvar, i32** @dllexportptr, align 4
+ ret void
+}
+

@llvm.global_ctors = appending constant

• [2 x { i32, void ()* }]

+ [3 x { i32, void ()* }]

[{ i32, void ()* } { i32 65535, void ()* @test1 },

• { i32, void ()* } { i32 65535, void ()* @test2 }]

+ { i32, void ()* } { i32 65535, void ()* @test2 },
+ { i32, void ()* } { i32 65535, void ()* @test3 }]

[nicholas]

(Ignore my comments on the previous revision of the patch. Sorry for the confusion there!)

[majnemer]

Closed by commit rL211571 (authored by @majnemer).

lib/Transforms/IPO/GlobalOpt.cpp

 /// because the code generator doesn't have a relocation that can handle that.
 ///
 /// This function should be called if C was not found (but just got inserted)
 /// in SimpleConstants to avoid having to rescan the same constants all the
 /// time.
 static bool isSimpleEnoughValueToCommitHelper(Constant *C,
                                    SmallPtrSet<Constant*, 8> &SimpleConstants,
                                    const DataLayout *DL) {
-  // Simple integer, undef, constant aggregate zero, global addresses, etc are
-  // all supported.
-  if (C->getNumOperands() == 0 || isa<BlockAddress>(C) ||
-      isa<GlobalValue>(C))
+  // Simple global addresses are supported, do not allow dllimport globals.
+  if (auto *GV = dyn_cast<GlobalValue>(C))
+    return !GV->hasDLLImportStorageClass();
+
+  // Simple integer, undef, constant aggregate zero, etc are all supported.
+  if (C->getNumOperands() == 0 || isa<BlockAddress>(C))
     return true;
 
   // Aggregate values are safe if all their elements are.
   if (isa<ConstantArray>(C) || isa<ConstantStruct>(C) ||
       isa<ConstantVector>(C)) {
     for (unsigned i = 0, e = C->getNumOperands(); i != e; ++i) {
       Constant *Op = cast<Constant>(C->getOperand(i));
       if (!isSimpleEnoughValueToCommit(Op, SimpleConstants, DL))

 /// globals.  This should be kept up to date with CommitValueTo.
 static bool isSimpleEnoughPointerToCommit(Constant *C) {
   // Conservatively, avoid aggregate types. This is because we don't
   // want to worry about them partially overlapping other stores.
   if (!cast<PointerType>(C->getType())->getElementType()->isSingleValueType())
     return false;
 
   if (GlobalVariable *GV = dyn_cast<GlobalVariable>(C))
-    // Do not allow weak/*_odr/linkonce/dllimport/dllexport linkage or
-    // external globals.
+    // Do not allow weak/*_odr/linkonce linkage or external globals.
     return GV->hasUniqueInitializer();
 
   if (ConstantExpr *CE = dyn_cast<ConstantExpr>(C)) {
     // Handle a constantexpr gep.
     if (CE->getOpcode() == Instruction::GetElementPtr &&
         isa<GlobalVariable>(CE->getOperand(0)) &&
         cast<GEPOperator>(CE)->isInBounds()) {
       GlobalVariable *GV = cast<GlobalVariable>(CE->getOperand(0));

test/Transforms/GlobalOpt/constantfold-initializers.ll

 define internal void @test2() {
 entry:
   store i32 4, i32* getelementptr inbounds (%closure* @f, i32 0, i32 0)
   %call = call i32 @test2_helper(%closure* @f, i32 9)
   store i32 %call, i32* @m, align 4
   ret void
 }
 
+; PR19955
+
+@dllexportptr = global i32* null, align 4
+; CHECK-NOT: @dllexportptr = global i32* @dllexportvar, align 4
+@dllexportvar = external dllimport global i32
+define internal void @test3() {
+entry:
+  store i32* @dllexportvar, i32** @dllexportptr, align 4
+  ret void
+}
+
 @llvm.global_ctors = appending constant
-  [2 x { i32, void ()* }]
+  [3 x { i32, void ()* }]
   [{ i32, void ()* } { i32 65535, void ()* @test1 },
-   { i32, void ()* } { i32 65535, void ()* @test2 }]
+   { i32, void ()* } { i32 65535, void ()* @test2 },
+   { i32, void ()* } { i32 65535, void ()* @test3 }]