This is an archive of the discontinued LLVM Phabricator instance.

GlobalOpt: Don't optimize dllimport for initializers
ClosedPublic

Authored by majnemer on Jun 22 2014, 9:23 PM.

Details

Summary

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

Diff Detail

Event Timeline

majnemer updated this revision to Diff 10734.Jun 22 2014, 9:23 PM
majnemer retitled this revision from to GlobalOpt: Don't optimize dllimport for initializers.
majnemer updated this object.
majnemer added reviewers: rnk, nicholas.
majnemer added a subscriber: Unknown Object (MLST).
majnemer updated this revision to Diff 10737.Jun 22 2014, 11:30 PM
  • Move this check to a more appropriate place.
rnk added inline comments.Jun 23 2014, 12:36 PM
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 updated this revision to Diff 10762.Jun 23 2014, 1:29 PM
  • Address review comments.
majnemer added inline comments.Jun 23 2014, 1:31 PM
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 edited edge metadata.Jun 23 2014, 11:37 PM

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 accepted this revision.Jun 23 2014, 11:58 PM
nicholas edited edge metadata.

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

This revision is now accepted and ready to land.Jun 23 2014, 11:58 PM
majnemer closed this revision.Jun 24 2014, 12:01 AM
majnemer updated this revision to Diff 10774.

Closed by commit rL211571 (authored by @majnemer).