[LTO] Fix commons handling

[mehdi_amini]

Summary

Previously the prevailing information was not honored, and commons
symbols could override a strong definition. This patch fixes it and
propose the following semantic for commons: the client should mark
as prevailing the commons that it expects the LTO implementation to
merge (i.e. take the maximum size and alignment).
It implies that commons are allowed to have multiple prevailing
definitions.

[pcc]

The problem with this solution is that it requires the client to report all instances of the common symbol as prevailing. gold reports a single instance of the symbol as prevailing, and it won't necessarily be the first one. So the gold plugin can't know whether to report any given non-prevailing common symbol as prevailing since it doesn't know whether the eventual prevailing instance will be common or not. That's what the CommonResolution::Prevailing bit was tracking in the original gold plugin code. I think we'll need something similar here.

[mehdi_amini]

Change the API contract: if a least one instance of a common symbol is marked as prevailing, make sure it has the max(size) and max(alignment) over all the common instance for this symbol.

[pcc]

lib/LTO/LTO.cpp

356

Please update this comment.

test/tools/llvm-lto2/X86/common.ll

57

Please also add a test with a single common marked as prevailing.

[mehdi_amini]

Update comment

[mehdi_amini]

test/tools/llvm-lto2/X86/common.ll

57

Isn't is already what LARGE-PREVAILED and SMALL-PREVAILED are?

[pcc]

LGTM

Isn't is already what LARGE-PREVAILED and SMALL-PREVAILED are?

Yes, sorry, I missed those.

One other thing: the test you are updating should live in test/LTO/Resolution/X86 because it is testing the API rather than the command line tool.

lib/LTO/LTO.cpp

if (Res.Prevailing && GV) {
         GV->setLinkage(GlobalValue::WeakAnyLinkage);
         break;
       case GlobalValue::LinkOnceODRLinkage:
         GV->setLinkage(GlobalValue::WeakODRLinkage);
         break;
       }
     }
     // Common resolution: collect the maximum size/alignment.
-    // FIXME: right now we ignore the prevailing information, it is not clear
-    // what is the "right" behavior here.
-    if (Sym.getFlags() & object::BasicSymbolRef::SF_Common) {
+    // We take the maximum Size/Alignment over all the prevailing Common.
+    // The client has to mark all the common as prevailing to take the max of
+    // all.
+    if (Res.Prevailing && Sym.getFlags() & object::BasicSymbolRef::SF_Common) {
       auto &CommonRes = RegularLTO.Commons[Sym.getIRName()];
       CommonRes.Size = std::max(CommonRes.Size, Sym.getCommonSize());
       CommonRes.Align = std::max(CommonRes.Align, Sym.getCommonAlignment());
     }
 
     // FIXME: use proposed local attribute for FinalDefinitionInLinkageUnit.
   }
   assert(ResI == Res.end());

test/LTO/Resolution/X86/Inputs/commons.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@x = global i32 42, align 4

test/LTO/Resolution/X86/commons.ll

+; RUN: llvm-as -o %t1.bc %s
+; RUN: llvm-as -o %t2.bc %p/Inputs/commons.ll
+; RUN: llvm-lto2 %t1.bc -r=%t1.bc,x,l %t2.bc -r=%t2.bc,x,pl -o %t.out -save-temps
+; RUN: llvm-dis -o - %t.out.0.0.preopt.bc  | FileCheck %s
+
+; A strong definition should override the common
+; CHECK: @x = global i32 42, align 4
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@x = common global i16 0, align 2

test/tools/llvm-lto2/X86/common.ll

 ; Same as before, but reversing the order of the inputs
 ; RUN: llvm-lto2 %t2.bc %t1.bc -o %t.o -save-temps \
 ; RUN:  -r %t1.bc,v,x \
 ; RUN:  -r %t2.bc,v,px \
 ; RUN:  -r %t1.bc,foo,px \
 ; RUN:  -r %t2.bc,bar,px
 ; RUN: llvm-dis < %t.o.0.0.preopt.bc | FileCheck %s --check-prefix=LARGE-PREVAILED
 
-
 ; Client marked the "small with large alignment" one as prevailing
 ; RUN: llvm-lto2 %t1.bc %t2.bc -o %t.o -save-temps \
 ; RUN:  -r %t1.bc,v,px \
 ; RUN:  -r %t2.bc,v,x \
 ; RUN:  -r %t1.bc,foo,px \
 ; RUN:  -r %t2.bc,bar,px
 ; RUN: llvm-dis < %t.o.0.0.preopt.bc | FileCheck %s --check-prefix=SMALL-PREVAILED
 

 ; Same as before, but reversing the order of the inputs
 ; RUN: llvm-lto2 %t2.bc %t1.bc -o %t.o -save-temps \
 ; RUN:  -r %t1.bc,v,x \
 ; RUN:  -r %t2.bc,v,x \
 ; RUN:  -r %t1.bc,foo,px \
 ; RUN:  -r %t2.bc,bar,px
 ; RUN: llvm-dis < %t.o.0.0.preopt.bc | FileCheck  %s --check-prefix=NONE-PREVAILED2
 
+
+
+; Client marked both as prevailing
+; RUN: llvm-lto2 %t1.bc %t2.bc -o %t.o -save-temps \
+; RUN:  -r %t1.bc,v,px \
+; RUN:  -r %t2.bc,v,px \
+; RUN:  -r %t1.bc,foo,px \
+; RUN:  -r %t2.bc,bar,px
+; RUN: llvm-dis < %t.o.0.0.preopt.bc | FileCheck %s --check-prefix=BOTH-PREVAILED1
+
+; Same as before, but reversing the order of the inputs
+; RUN: llvm-lto2 %t2.bc %t1.bc -o %t.o -save-temps \
+; RUN:  -r %t1.bc,v,px \
+; RUN:  -r %t2.bc,v,px \
+; RUN:  -r %t1.bc,foo,px \
+; RUN:  -r %t2.bc,bar,px
+; RUN: llvm-dis < %t.o.0.0.preopt.bc | FileCheck %s --check-prefix=BOTH-PREVAILED2
+
+
+
 target triple = "x86_64-apple-macosx10.11.0"
 
 @v = common global i8 0, align 8
 
-; LARGE-PREVAILED: @v = common global i16 0, align 8
-; SMALL-PREVAILED: @v = common global [2 x i8] zeroinitializer, align 8
-; In this case the first was kept as external, but we created a new merged
-; common due to the second requiring a larger size:
-; NONE-PREVAILED1: @v = common global [2 x i8] zeroinitializer, align 8
-; NONE-PREVAILED2: @v = external global i16, align 8
+; LARGE-PREVAILED: @v = common global i16 0, align 4
+; SMALL-PREVAILED: @v = common global i8 0, align 8
+; BOTH-PREVAILED1: @v = common global i16 0, align 8
+; BOTH-PREVAILED2: common global [2 x i8] zeroinitializer, align 8
+; In this case the first is kept as external
+; NONE-PREVAILED1: @v = external global i8, align 8
+; NONE-PREVAILED2: @v = external global i16, align 4
 
 define i8 *@foo() {
  ret i8 *@v
 }