[llvm] r291304 - [ThinLTO] Handle conflicting local names gracefully

[Teresa Johnson via llvm-commits]

Author: tejohnson
Date: Fri Jan  6 17:38:41 2017
New Revision: 291304

URL: http://llvm.org/viewvc/llvm-project?rev=291304&view=rev
Log:
[ThinLTO] Handle conflicting local names gracefully

Summary:
r285871 introduced an assert that was overly aggressive in the case
of a same-named local in different same-named files (in different
directories), where the source name and therefore the GUID ended up
the same because the files were compiled in their own directory without
any leading path. Change the handling in the promotion logic to get
the summary for the version in that module.

This also exposed an issue where we are not always importing the
right copy, which is a performance not correctness issue (because
the renaming is based on the module hash which must be different,
see the bug report for details). I will fix that as a follow-on.

Fixes PR31561.

Reviewers: mehdi_amini

Subscribers: llvm-commits

Differential Revision: https://reviews.llvm.org/D28411

Added:
    llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict1.ll
    llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict2.ll
    llvm/trunk/test/ThinLTO/X86/local_name_conflict.ll
Modified:
    llvm/trunk/lib/Transforms/Utils/FunctionImportUtils.cpp

Modified: llvm/trunk/lib/Transforms/Utils/FunctionImportUtils.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/FunctionImportUtils.cpp?rev=291304&r1=291303&r2=291304&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Utils/FunctionImportUtils.cpp (original)
+++ llvm/trunk/lib/Transforms/Utils/FunctionImportUtils.cpp Fri Jan  6 17:38:41 2017
@@ -67,12 +67,15 @@ bool FunctionImportGlobalProcessing::sho
     return true;
   }
 
-  // When exporting, consult the index.
-  auto Summaries = ImportIndex.findGlobalValueSummaryList(SGV->getGUID());
-  assert(Summaries != ImportIndex.end() &&
-         "Missing summary for global value when exporting");
-  assert(Summaries->second.size() == 1 && "Local has more than one summary");
-  auto Linkage = Summaries->second.front()->linkage();
+  // When exporting, consult the index. We can have more than one local
+  // with the same GUID, in the case of same-named locals in different but
+  // same-named source files that were compiled in their respective directories
+  // (so the source file name and resulting GUID is the same). Find the one
+  // in this module.
+  auto Summary = ImportIndex.findSummaryInModule(
+      SGV->getGUID(), SGV->getParent()->getModuleIdentifier());
+  assert(Summary && "Missing summary for global value when exporting");
+  auto Linkage = Summary->linkage();
   if (!GlobalValue::isLocalLinkage(Linkage)) {
     assert(!isNonRenamableLocal(*SGV) &&
            "Attempting to promote non-renamable local");

Added: llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict1.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict1.ll?rev=291304&view=auto
==============================================================================
--- llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict1.ll (added)
+++ llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict1.ll Fri Jan  6 17:38:41 2017
@@ -0,0 +1,17 @@
+; ModuleID = 'local_name_conflict.o'
+source_filename = "local_name_conflict.c"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: noinline nounwind uwtable
+define i32 @a() {
+entry:
+  %call = call i32 @foo()
+  ret i32 %call
+}
+
+; Function Attrs: noinline nounwind uwtable
+define internal i32 @foo() {
+entry:
+  ret i32 1
+}

Added: llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict2.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict2.ll?rev=291304&view=auto
==============================================================================
--- llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict2.ll (added)
+++ llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict2.ll Fri Jan  6 17:38:41 2017
@@ -0,0 +1,17 @@
+; ModuleID = 'local_name_conflict.o'
+source_filename = "local_name_conflict.c"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: noinline nounwind uwtable
+define i32 @b() {
+entry:
+  %call = call i32 @foo()
+  ret i32 %call
+}
+
+; Function Attrs: noinline nounwind uwtable
+define internal i32 @foo() {
+entry:
+  ret i32 2
+}

Added: llvm/trunk/test/ThinLTO/X86/local_name_conflict.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/ThinLTO/X86/local_name_conflict.ll?rev=291304&view=auto
==============================================================================
--- llvm/trunk/test/ThinLTO/X86/local_name_conflict.ll (added)
+++ llvm/trunk/test/ThinLTO/X86/local_name_conflict.ll Fri Jan  6 17:38:41 2017
@@ -0,0 +1,29 @@
+; Do setup work for all below tests: generate bitcode and combined index
+; RUN: opt -module-summary -module-hash %s -o %t.bc
+; RUN: opt -module-summary -module-hash %p/Inputs/local_name_conflict1.ll -o %t2.bc
+; RUN: opt -module-summary -module-hash %p/Inputs/local_name_conflict2.ll -o %t3.bc
+; RUN: llvm-lto -thinlto-action=thinlink -o %t4.bc %t.bc %t2.bc %t3.bc
+
+; Make sure foo is promoted and renamed without complaint in both
+; Inputs/local_name_conflict1.ll and Inputs/local_name_conflict2.ll
+; FIXME: Once the importer is fixed to import the correct copy of the
+; local, we should be able to verify that via an import action.
+; RUN: llvm-lto -thinlto-action=promote %t2.bc -thinlto-index=%t4.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=EXPORTSTATIC
+; RUN: llvm-lto -thinlto-action=promote %t3.bc -thinlto-index=%t4.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=EXPORTSTATIC
+; EXPORTSTATIC: define hidden i32 @foo.llvm.
+
+; ModuleID = 'local_name_conflict_main.o'
+source_filename = "local_name_conflict_main.c"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: noinline nounwind uwtable
+define i32 @main() {
+entry:
+  %retval = alloca i32, align 4
+  store i32 0, i32* %retval, align 4
+  %call = call i32 (...) @b()
+  ret i32 %call
+}
+
+declare i32 @b(...)