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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:1485
   DenseMap<StringRef, ExportedFunctionInfo> ExportedFunctions;
   if (ExportSummary) {
     NamedMDNode *CfiFunctionsMD = M.getNamedMetadata("cfi.functions");
----------------
tejohnson wrote:
> 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).
Yes, ExportSummary is for full LTO. It's documented here: http://llvm-cs.pcc.me.uk/include/llvm/Transforms/IPO/PassManagerBuilder.h#138

In a separate change I'll add a reference to that where those fields are defined.


================
Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:1515
 
-        if (Linkage == CFL_Definition)
-          F->eraseMetadata(LLVMContext::MD_type);
----------------
tejohnson wrote:
> 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.
> I'm assuming this change is to handle your first case:
Yes.

> Also, is M the full LTO module or the ThinLTO Module?
It is the full LTO module.

> I'm a bit confused on where here indicates that the LTO version is prevailing and there is a non-prevailing def in ThinLTO? 
If the LTO def is prevailing and the ThinLTO def is non-prevailing, we will see a function definition here (because it will have been copied into the combined module by the IRMover) as well as a definition entry in cfi.functions for that function (because we will have added it to the full LTO module in the ThinLTO bitcode writer -- we know it is non-prevailing because we can expect to see max 1 prevailing definition per symbol).

It looks like the original code assumed that if cfi.functions contained a definition entry for a function, there must not be a definition in the full LTO module (and therefore we can go ahead and drop the metadata and expect to enter the if statement below to re-add it). But that wasn't accounting for the ThinLTO def potentially being non-prevailing.


================
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);
----------------
pcc wrote:
> tejohnson wrote:
> > 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.
> > I'm assuming this change is to handle your first case:
> Yes.
> 
> > Also, is M the full LTO module or the ThinLTO Module?
> It is the full LTO module.
> 
> > I'm a bit confused on where here indicates that the LTO version is prevailing and there is a non-prevailing def in ThinLTO? 
> If the LTO def is prevailing and the ThinLTO def is non-prevailing, we will see a function definition here (because it will have been copied into the combined module by the IRMover) as well as a definition entry in cfi.functions for that function (because we will have added it to the full LTO module in the ThinLTO bitcode writer -- we know it is non-prevailing because we can expect to see max 1 prevailing definition per symbol).
> 
> It looks like the original code assumed that if cfi.functions contained a definition entry for a function, there must not be a definition in the full LTO module (and therefore we can go ahead and drop the metadata and expect to enter the if statement below to re-add it). But that wasn't accounting for the ThinLTO def potentially being non-prevailing.
Will do.


================
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
----------------
tejohnson wrote:
> Are the changes in this test case testing for both parts of the fix in this patch?
Hmm, not quite. There ought to also be a test with a linker definition here. I'll add it.


================
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]]
----------------
tejohnson wrote:
> Without the fix in this patch I guess this would try to create an alias with available_externally linkage?
Right.


https://reviews.llvm.org/D35604