[PATCH] D35604: LowerTypeTests: Drop function type metadata only if we're going to replace it.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:1485
   DenseMap<StringRef, ExportedFunctionInfo> ExportedFunctions;
   if (ExportSummary) {
     NamedMDNode *CfiFunctionsMD = M.getNamedMetadata("cfi.functions");
----------------
ExportSummary is the full LTO case? Can you note that somewhere? I forget where that is documented (it isn't where ExportSummary and ImportSummary are declared in LowerTypeTestsModule and LowerTypeTests).


================
Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:1515
 
-        if (Linkage == CFL_Definition)
-          F->eraseMetadata(LLVMContext::MD_type);
----------------
I'm assuming this change is to handle your first case:
"jump table members with a prevailing
definition in a full LTO module and a non-prevailing definition in a
ThinLTO module...This patch causes us to drop metadata only if we are about to replace
it with metadata from cfi.functions."

I'm a bit confused on where here indicates that the LTO version is prevailing and there is a non-prevailing def in ThinLTO? Also, is M the full LTO module or the ThinLTO Module? I was assuming the latter but since F is a declaration below, it wouldn't be the case where F was prevailing in this module.


================
Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:1515
 
-        if (Linkage == CFL_Definition)
-          F->eraseMetadata(LLVMContext::MD_type);
+        if (F->hasAvailableExternallyLinkage()) {
+          F->setLinkage(GlobalValue::ExternalLinkage);
----------------
tejohnson wrote:
> I'm assuming this change is to handle your first case:
> "jump table members with a prevailing
> definition in a full LTO module and a non-prevailing definition in a
> ThinLTO module...This patch causes us to drop metadata only if we are about to replace
> it with metadata from cfi.functions."
> 
> I'm a bit confused on where here indicates that the LTO version is prevailing and there is a non-prevailing def in ThinLTO? Also, is M the full LTO module or the ThinLTO Module? I was assuming the latter but since F is a declaration below, it wouldn't be the case where F was prevailing in this module.
Needs comment.


================
Comment at: llvm/test/Transforms/LowerTypeTests/export-icall.ll:1
 ; RUN: opt -S -lowertypetests -lowertypetests-summary-action=export -lowertypetests-read-summary=%S/Inputs/use-typeid1-typeid2.yaml -lowertypetests-write-summary=%t < %s | FileCheck %s
 ; RUN: FileCheck --check-prefix=SUMMARY %s < %t
----------------
Are the changes in this test case testing for both parts of the fix in this patch?


================
Comment at: llvm/test/Transforms/LowerTypeTests/export-icall.ll:41
 ; CHECK-DAG: @f                    = alias void (i32), {{.*}}getelementptr {{.*}}void ()* [[JT1]]
+; CHECK-DAG: @f2                   = alias void (i32), {{.*}}getelementptr {{.*}}void ()* [[JT1]]
 ; CHECK-DAG: @external.cfi_jt      = hidden alias void (), {{.*}}getelementptr {{.*}}void ()* [[JT1]]
----------------
Without the fix in this patch I guess this would try to create an alias with available_externally linkage?


https://reviews.llvm.org/D35604