[HCC] Add flag to Import Weak Functions in Function Importer

[ashi1]

Summary

For HCC and HIP, in order to run kernels on our GPUs, we need everything to be inlined. We are moving towards ThinLTO implementation of the Link-time. To take advantage of ThinLTO's cross-module function importing, we must have all functions imported to kernel files including weak functions.

Since Function Importing prevents importing weak functions due to ambiguity, we would like to set a flag which can force weak functions to be imported as well.

[yaxunl]

Could you please upload a diff with full context? Also, need a lit test.

[ashi1]

I've added the lit tests for this change, and also showing full context.

My lit test import_weak_type.ll follows similar format to import_opaque_type.ll.

[yaxunl]

lib/Transforms/IPO/FunctionImport.cpp

107

Is it possible not to expose this option through extern? Generally these options should be kept static.

[AlexVlx]

lib/Transforms/IPO/FunctionImport.cpp

107

This seems to suggest that it is not utterly unacceptable: http://llvm.org/docs/CommandLine.html#internal-vs-external-storage, and the use case described there maps pretty closely to this, since the flag itself must be accessible from two different TUs. Am I missing something?

[yaxunl]

lib/Transforms/IPO/FunctionImport.cpp

107

OK. I did not notice that it is also used byFunctionImportUtils.cpp.

lib/Transforms/Utils/FunctionImportUtils.cpp

153

If we link three modules, A, B, and C.

A calls function sin, B and C both contains function sin with weak linkage.

Which sin function will be linked?

[b-sumner]

The usual rule is to take the first weak definition encountered.

[yaxunl]

In D39739#945318, @b-sumner wrote:

The usual rule is to take the first weak definition encountered.

Will this work for us? Usually we would like the last one to be linked.

[ashi1]

Is first one encountered a poor design?

[b-sumner]

In D39739#1045611, @ashi1 wrote:

Is first one encountered a poor design?

Strong or first weak is the standard behavior for ISA level linkers.

include/llvm/Transforms/Utils/FunctionImportUtils.h

 #define LLVM_TRANSFORMS_UTILS_FUNCTIONIMPORTUTILS_H
 
 #include "llvm/ADT/SetVector.h"
 #include "llvm/IR/ModuleSummaryIndex.h"
 
 namespace llvm {
 class Module;
 
+// Set to true depending on option -force-import-weak
+extern bool ForceImportWeakFlag;
+
 /// Class to handle necessary GlobalValue changes required by ThinLTO
 /// function importing, including linkage changes and any necessary renaming.
 class FunctionImportGlobalProcessing {
   /// The Module which we are exporting or importing functions from.
   Module &M;
 
   /// Module summary index passed in for function importing/exporting handling.
   const ModuleSummaryIndex &ImportIndex;

lib/Transforms/IPO/FunctionImport.cpp

static cl::opt<float> ImportColdMultiplier(
     cl::desc("Multiply the `import-instr-limit` threshold for cold callsites"));
 
 static cl::opt<bool> PrintImports("print-imports", cl::init(false), cl::Hidden,
                                   cl::desc("Print imported functions"));
 
 static cl::opt<bool> ComputeDead("compute-dead", cl::init(true), cl::Hidden,
                                  cl::desc("Compute dead symbols"));
 
+bool llvm::ForceImportWeakFlag;
+static cl::opt<bool, true>
+ForceImportWeak("force-import-weak", cl::Hidden,
+                cl::desc("Allow weak functions to be imported"),
+                cl::location(ForceImportWeakFlag), cl::init(false));
+
 static cl::opt<bool> EnableImportMetadata(
     "enable-import-metadata", cl::init(
 #if !defined(NDEBUG)
                                   true /*Enabled with asserts.*/
 #else
                                   false
 #endif
                                   ),
auto It = llvm::find_if(
         // in the index.
         // 2) There is a static variable with the  OriginalGUID identical
         // to the GUID of the library function in 1);
         // When this happens, the logic for SamplePGO kicks in and
         // the static variable in 2) will be found, which needs to be
         // filtered out.
         if (GVSummary->getSummaryKind() == GlobalValueSummary::GlobalVarKind)
           return false;
-        if (GlobalValue::isInterposableLinkage(GVSummary->linkage()))
+        if (!ForceImportWeakFlag && GlobalValue::isInterposableLinkage(GVSummary->linkage()))
           // There is no point in importing these, we can't inline them
           return false;
         if (isa<AliasSummary>(GVSummary))
           // Aliases can't point to "available_externally".
           // FIXME: we should import alias as available_externally *function*,
           // the destination module does not need to know it is an alias.
           return false;
 

lib/Transforms/Utils/FunctionImportUtils.cpp

if (!doImportAsDefinition(SGV))
       return GlobalValue::ExternalLinkage;
     // An imported available_externally declaration stays that way.
     return SGV->getLinkage();
 
   case GlobalValue::WeakAnyLinkage:
     // Can't import weak_any definitions correctly, or we might change the
     // program semantics, since the linker will pick the first weak_any
     // definition and importing would change the order they are seen by the
     // linker. The module linking caller needs to enforce this.
+    if(!ForceImportWeakFlag)
       assert(!doImportAsDefinition(SGV));
     // If imported as a declaration, it becomes external_weak.
     return SGV->getLinkage();
 
   case GlobalValue::WeakODRLinkage:
     // For weak_odr linkage, there is a guarantee that all copies will be
     // equivalent, so the issue described above for weak_any does not exist,
     // and the definition can be imported. It can be treated similarly
     // to an imported externally visible global value.

test/ThinLTO/X86/Inputs/import_weak_type.ll

+target datalayout = "e-p:64:64-p1:64:64-p2:64:64-p3:32:32-p4:32:32-p5:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-A5"
+target triple = "amdgcn--amdhsa-hcc"
+
+define weak void @foo() {
+  ret void
+}
+

test/ThinLTO/X86/import_weak_type.ll

+; Do setup work for all below tests: generate bitcode and combined index
+; RUN: opt -module-summary %s -o %t.bc
+; RUN: opt -module-summary %p/Inputs/import_weak_type.ll -o %t2.bc
+; RUN: llvm-lto -thinlto-action=thinlink -o %t3.bc %t.bc %t2.bc
+
+; Check that we import correctly the imported weak type to replace declaration here
+; RUN: llvm-lto -thinlto-action=import %t.bc -thinlto-index=%t3.bc -force-import-weak -o - | llvm-dis -o - | FileCheck %s
+; CHECK: define weak void @foo()
+
+
+target datalayout = "e-p:64:64-p1:64:64-p2:64:64-p3:32:32-p4:32:32-p5:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-A5"
+target triple = "amdgcn--amdhsa-hcc"
+
+declare extern_weak void @foo()
+define weak_odr amdgpu_kernel void @main() {
+    call void @foo()
+  ret void
+}
+