[GlobalMerge] Don't merge dllexport globals

[mstorsjo]

Summary

Merging such globals loses the dllexport attribute.

[john.brawn]

Why not only disable merging of dllexport variables (in GlobalMerge::doInitialization, where we already disable it on various kinds of variables), instead of all variables?

[mstorsjo]

In D42127#980273, @john.brawn wrote:

Why not only disable merging of dllexport variables (in GlobalMerge::doInitialization, where we already disable it on various kinds of variables), instead of all variables?

That probably works as well (or even trying to preserve the attribute?). I'll give it a try.

[mstorsjo]

Changed to explicitly check for the dllexport attribute.

[compnerd]

Can you add a test case to ensure that it still triggers for non-dllexport cases with this target?

[mstorsjo]

Added a testcase to show that normal globals still are merged.

[mstorsjo]

Ping @john.brawn and @compnerd - does it look good to you now?

[compnerd]

Wouldn't it be possible to merge multiple dllexport globals?

@x = dllexport global i32 0, align 4
@y = dllexport global i32 0, align 4
@z = global i32 0, align 4
@a = global i32 0, align 4

could still merge into:

@x = dllexport global i32 0, align 4
@z = global i32 0, align 4

even though technically we should be able to merge all of that into a single global and preserve the dllexport attribute.

[mstorsjo]

In D42127#985263, @compnerd wrote:

Wouldn't it be possible to merge multiple dllexport globals?

@x = dllexport global i32 0, align 4
@y = dllexport global i32 0, align 4
@z = global i32 0, align 4
@a = global i32 0, align 4

could still merge into:

@x = dllexport global i32 0, align 4
@z = global i32 0, align 4

even though technically we should be able to merge all of that into a single global and preserve the dllexport attribute.

It could be doable, but it's rather much work - way more than this patch at least.

I've looked at what's missing - it'd require adding the dllstorage enum/field from GlobalValue into GlobalAlias (not sure if this requires extending the bitcode format), hook up GlobalAlias to the emitLinkerFlagsForGlobal function. I'd like to have the bug fixed in 6.0 as well, and this feels more and more out of scope for backporting.

IMO it'd be better to go for the approach of this patch first and backport that, and then look at maybe adding dllstorage to GlobalAlias for trunk after that, if actually doable.

[compnerd]

That sounds reasonable. Please file PRs for the follow up work to enable the global merge for the dll export case.

[mstorsjo]

@hans - can we backport this to the 6.0 branch? The usecase it fixes is "compiling Qt for Windows on ARM" (which previously fails when building with -O3). The fix in SVN r323307 probably requires merging SVN r322900 as well, to go without conflicts - it's a trivial testcase touch-up.

[hans]

In D42127#986067, @mstorsjo wrote:

@hans - can we backport this to the 6.0 branch? The usecase it fixes is "compiling Qt for Windows on ARM" (which previously fails when building with -O3). The fix in SVN r323307 probably requires merging SVN r322900 as well, to go without conflicts - it's a trivial testcase touch-up.

Merged in r323337.

llvm/trunk/lib/CodeGen/GlobalMerge.cpp

DenseMap<unsigned, SmallVector<GlobalVariable *, 16>> Globals, ConstGlobals,
                                                         BSSGlobals;
   bool Changed = false;
   setMustKeepGlobalVariables(M);
 
   // Grab all non-const globals.
   for (auto &GV : M.globals()) {
     // Merge is safe for "normal" internal or external globals only
     if (GV.isDeclaration() || GV.isThreadLocal() ||
-        GV.hasSection() || GV.hasImplicitSection())
+        GV.hasSection() || GV.hasImplicitSection() ||
+        GV.hasDLLExportStorageClass())
       continue;
 
     // It's not safe to merge globals that may be preempted
     if (TM && !TM->shouldAssumeDSOLocal(M, &GV))
       continue;
 
     if (!(MergeExternalGlobals && GV.hasExternalLinkage()) &&
         !GV.hasInternalLinkage())

llvm/trunk/test/CodeGen/ARM/global-merge-dllexport.ll

+; RUN: llc < %s -mtriple=thumbv7-win32 -arm-global-merge | FileCheck %s
+
+@x = global i32 0, align 4
+@y = dllexport global i32 0, align 4
+
+define void @f1(i32 %a1, i32 %a2) {
+; CHECK: f1:
+; CHECK: movw [[REG1:r[0-9]+]], :lower16:x
+; CHECK: movt [[REG1]], :upper16:x
+  store i32 %a1, i32* @x, align 4
+  store i32 %a2, i32* @y, align 4
+  ret void
+}
+
+; CHECK-NOT: .L_MergedGlobals

llvm/trunk/test/CodeGen/ARM/global-merge-external.ll

 ; RUN: llc < %s -mtriple=arm-eabi  -arm-global-merge                                 | FileCheck %s --check-prefixes=CHECK,CHECK-MERGE
 ; RUN: llc < %s -mtriple=arm-eabi  -arm-global-merge -global-merge-on-external=true  | FileCheck %s --check-prefixes=CHECK,CHECK-MERGE
 ; RUN: llc < %s -mtriple=arm-eabi  -arm-global-merge -global-merge-on-external=false | FileCheck %s --check-prefixes=CHECK,CHECK-NO-MERGE
 ; RUN: llc < %s -mtriple=arm-macho -arm-global-merge                                 | FileCheck %s --check-prefixes=CHECK,CHECK-NO-MERGE
 ; RUN: llc < %s -mtriple=arm-eabi  -arm-global-merge -relocation-model=pic           | FileCheck %s --check-prefixes=CHECK,CHECK-NO-MERGE
+; RUN: llc < %s -mtriple=thumbv7-win32 -arm-global-merge                             | FileCheck %s --check-prefixes=CHECK-WIN32
 
 @x = global i32 0, align 4
 @y = global i32 0, align 4
 @z = global i32 0, align 4
 
 define void @f1(i32 %a1, i32 %a2) {
 ;CHECK:          f1:
 ;CHECK:          ldr {{r[0-9]+}}, [[LABEL1:\.?LCPI[0-9]+_[0-9]]]
 ;CHECK:          [[LABEL1]]:
 ;CHECK-MERGE:    .long .L_MergedGlobals
 ;CHECK-NO-MERGE: .long {{_?x}}
+;CHECK-WIN32:    f1:
+;CHECK-WIN32:    movw [[REG1:r[0-9]+]], :lower16:.L_MergedGlobals
+;CHECK-WIN32:    movt [[REG1]], :upper16:.L_MergedGlobals
   store i32 %a1, i32* @x, align 4
   store i32 %a2, i32* @y, align 4
   ret void
 }
 
 define void @g1(i32 %a1, i32 %a2) {
 ;CHECK:          g1:
 ;CHECK:          ldr {{r[0-9]+}}, [[LABEL2:\.?LCPI[0-9]+_[0-9]]]
 ;CHECK:          [[LABEL2]]:
 ;CHECK-MERGE:    .long .L_MergedGlobals
 ;CHECK-NO-MERGE: .long {{_?y}}
+;CHECK-WIN32:    g1:
+;CHECK-WIN32:    movw [[REG2:r[0-9]+]], :lower16:.L_MergedGlobals
+;CHECK-WIN32:    movt [[REG2]], :upper16:.L_MergedGlobals
   store i32 %a1, i32* @y, align 4
   store i32 %a2, i32* @z, align 4
   ret void
 }
 
 ;CHECK-NO-MERGE-NOT: .globl .L_MergedGlobals
 
 ;CHECK-MERGE: .type .L_MergedGlobals,%object
 ;CHECK-MERGE: .local  .L_MergedGlobals
 ;CHECK-MERGE: .comm .L_MergedGlobals,12,4
+;CHECK-WIN32: .lcomm  .L_MergedGlobals,12,4
 
 ;CHECK-MERGE: .globl  x
 ;CHECK-MERGE: x = .L_MergedGlobals
 ;CHECK-MERGE: .size x, 4
 ;CHECK-MERGE: .globl  y
 ;CHECK-MERGE: y = .L_MergedGlobals+4
 ;CHECK-MERGE: .size y, 4
 ;CHECK-MERGE: .globl  z
 ;CHECK-MERGE: z = .L_MergedGlobals+8
 ;CHECK-MERGE: .size z, 4
+
+;CHECK-WIN32: .globl  x
+;CHECK-WIN32: x = .L_MergedGlobals
+;CHECK-WIN32: .globl  y
+;CHECK-WIN32: y = .L_MergedGlobals+4
+;CHECK-WIN32: .globl  z
+;CHECK-WIN32: z = .L_MergedGlobals+8