[PATCH] D24545: [LTO] Fix commons handling

[Mehdi AMINI via llvm-commits]

mehdi_amini created this revision.
mehdi_amini added a reviewer: pcc.
mehdi_amini added a subscriber: llvm-commits.
Herald added a subscriber: mehdi_amini.

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.

https://reviews.llvm.org/D24545

Files:
  lib/LTO/LTO.cpp
  test/LTO/Resolution/X86/Inputs/commons.ll
  test/LTO/Resolution/X86/commons.ll
  test/tools/llvm-lto2/X86/common.ll

Index: test/tools/llvm-lto2/X86/common.ll
===================================================================
--- test/tools/llvm-lto2/X86/common.ll
+++ test/tools/llvm-lto2/X86/common.ll
@@ -19,7 +19,6 @@
 ; 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 \
@@ -53,16 +52,37 @@
 ; 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
Index: test/LTO/Resolution/X86/commons.ll
===================================================================
--- /dev/null
+++ test/LTO/Resolution/X86/commons.ll
@@ -0,0 +1,12 @@
+; 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"
+
+ at x = common global i16 0, align 2
Index: test/LTO/Resolution/X86/Inputs/commons.ll
===================================================================
--- /dev/null
+++ test/LTO/Resolution/X86/Inputs/commons.ll
@@ -0,0 +1,4 @@
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+ at x = global i32 42, align 4
Index: lib/LTO/LTO.cpp
===================================================================
--- lib/LTO/LTO.cpp
+++ lib/LTO/LTO.cpp
@@ -353,9 +353,10 @@
       }
     }
     // 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());


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D24545.71296.patch
Type: text/x-patch
Size: 3915 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160914/fba807e8/attachment.bin>