This is an archive of the discontinued LLVM Phabricator instance.

Differential D66814

[IRMover] Don't map globals if their types are the same
ClosedPublic

Authored by pirama on Aug 27 2019, 11:15 AM.

Details

Reviewers
tejohnson
pcc
srhines
Commits
rG8b46544641ef: [IRMover] Don't map globals if their types are the same
rL371643: [IRMover] Don't map globals if their types are the same
Summary

During IR Linking, if the types of two globals in destination and source
modules are the same, it can only be because the global in the
destination module is originally from the source module and got added to
the destination module from a shared metadata.




We shouldn't map this type to itself in case the type's components get

remapped to a new type from the destination (for instance, during the

loop over SrcM->getIdentifiedStructTypes() further below in

IRLinker::computeTypeMapping()).



Fixes PR40312.
Diff Detail
Repository 
rL LLVM 
Event Timeline
pirama created this revision.Aug 27 2019, 11:15 AM
Herald added a project: Restricted Project.  ·  View Herald TranscriptAug 27 2019, 11:15 AM
Herald added subscribers: dexonsmith, steven_wu, hiraditya, mehdi_amini.  ·  View Herald Transcript
Harbormaster completed remote builds in B37371: Diff 217441.Aug 27 2019, 11:16 AM
tejohnson added inline comments.Aug 27 2019, 11:52 AM
llvm/lib/Linker/IRMover.cpp


765 ↗(On Diff #217441)
Can you confirm how the DGV was added to the DstM originally? Do you mean it was already imported from SGV based on information in the summary? We don't directly add from the module summary, it is just used to compute the list of GVs to import from a source here via the IRMover.
pirama updated this revision to Diff 217495.Aug 27 2019, 2:27 PM
pirama marked an inline comment as done.
Fix typo reffers -> refers
llvm/lib/Linker/IRMover.cpp


765 ↗(On Diff #217441)
Yes, even without this change, DGV was added to DstM.



Here's the path that I traced inside a debugger:



Inputs/type-mapping-bug3.ll: function 'a' has attached debug metadata !6

Parsing that metadata ends up at metadata !5 (which is shared between both input files)

In type-mapping-bug3.ll, processing !5 leads to !7 which is a ConstantAsMetadata, with value as the function declaration.



This value/declaration gets added to DstM in Mapper::mapSimpleMetadata in ValueMapper.cpp.
Harbormaster completed remote builds in B37379: Diff 217495.Aug 27 2019, 2:28 PM
pirama marked an inline comment as done.Aug 27 2019, 8:40 PM
pirama added inline comments.
llvm/lib/Linker/IRMover.cpp


765 ↗(On Diff #217441)
Btw, this (GlobalValue from SrcM already added to DstM) happens for PR37684 (test/LTO/X86/type-mapping-bug2.ll) as well.  It's just that the types get appropriately mapped if this type and component types don't involve opaque types.  This fix is to handle the case where opaque types are involved.



Also, skipping the mapping here is at worst a delayed optimization since they should get resolved appropriately via TypeMapper::get.
tejohnson added inline comments.Aug 29 2019, 2:16 PM
llvm/lib/Linker/IRMover.cpp


765 ↗(On Diff #217441)
I still don't understand the comment about the module summary, which is just telling the IRMover which symbols to move. What are DGV and SGV in the case where the types match?


llvm/test/LTO/X86/Inputs/type-mapping-bug3.ll


9 ↗(On Diff #217495)
Second sentence is incomplete.


llvm/test/LTO/X86/type-mapping-bug3.ll


41 ↗(On Diff #217495)
Comment seems stale - there is no !11


45 ↗(On Diff #217495)
%t0.o?
vlad.tsyrklevich added a subscriber: vlad.tsyrklevich.Sep 4 2019, 3:49 AM
pirama updated this revision to Diff 218787.Sep 4 2019, 2:04 PM
Update/clean-up comments in the test.
Harbormaster completed remote builds in B37744: Diff 218787.Sep 4 2019, 2:04 PM
pirama marked 4 inline comments as done.Sep 4 2019, 2:06 PM
pirama added inline comments.
llvm/lib/Linker/IRMover.cpp


765 ↗(On Diff #217441)
I incorrectly used "module summary" in the patch.  The value is getting loaded via a shared metadata node (!5 in both test inputs)
pirama marked an inline comment as not done.Sep 4 2019, 2:06 PM
pirama added a comment.Sep 11 2019, 9:58 AM
Ping...



@tejohnson Did my reply about shared metadata clarify your question?
tejohnson added a comment.Sep 11 2019, 10:24 AM


In D66814#1666457, @pirama wrote:


Ping...



@tejohnson Did my reply about shared metadata clarify your question?





It makes more sense to me (note the summary description of the patch needs to be adjusted similarly, it still refers to the summary). I have a couple more questions below.
llvm/lib/Linker/IRMover.cpp


765 ↗(On Diff #217441)
In the test SGV and DGV are @d?


llvm/test/LTO/X86/Inputs/type-mapping-bug3.ll


23 ↗(On Diff #218787)
"refers to by" doesn't make sense. Do you mean "refers to !5 in..."?


llvm/test/LTO/X86/type-mapping-bug3.ll


3 ↗(On Diff #218787)
It's a little odd to say that the copy of d from this module is prevailing, since it is just a declaration. Can this be "%t1.o,d," and would that still trigger the error without your fix?


20 ↗(On Diff #218787)
"when its type is mapped to itself incorrectly"?
pirama marked 4 inline comments as done.Sep 11 2019, 11:02 AM


In D66814#1666487, @tejohnson wrote:




In D66814#1666457, @pirama wrote:







(note the summary description of the patch needs to be adjusted similarly, it still refers to the summary).





Aah, phabricator is not showing my updated commit message.  I'll manually update the summary.
llvm/lib/Linker/IRMover.cpp


765 ↗(On Diff #217441)
Yes.


llvm/test/LTO/X86/type-mapping-bug3.ll


3 ↗(On Diff #218787)
Yes, the error is triggered by %t1.o,d,.  I've updated the test to %t1.o,d, since that seems the preferred input.
pirama updated this revision to Diff 219744.Sep 11 2019, 11:03 AM
    

  • Do not have a declaration as a prevailing symbol in llvm-lto2's arguments
    • 

  • Fix comments in test
    • 

pirama edited the summary of this revision. (Show Details)Sep 11 2019, 11:04 AM
Harbormaster completed remote builds in B38019: Diff 219744.Sep 11 2019, 11:06 AM
tejohnson accepted this revision.Sep 11 2019, 11:09 AM
lgtm
This revision is now accepted and ready to land.Sep 11 2019, 11:09 AM
Closed by commit rL371643: [IRMover] Don't map globals if their types are the same (authored by pirama).  ·  Explain WhySep 11 2019, 11:35 AM
This revision was automatically updated to reflect the committed changes.
ychen mentioned this in D87001: [IRMover] Avoid materializing global value that belongs to not-yet-linked module.Sep 1 2020, 11:05 PM
ychen mentioned this in rGd9881e6e27bc: [IRMover] Avoid materializing global value that belongs to not-yet-linked module.Oct 7 2020, 6:23 PM
Revision Contents
PathSize
llvm/
trunk/
lib/
Linker/
12 lines
test/
LTO/
X86/
Inputs/
26 lines
47 lines
Diff 219752
llvm/trunk/lib/Linker/IRMover.cpp
Show First 20 Lines • Show All 752 Lines • ▼ Show 20 Lines  for (GlobalValue &SGV : SrcM->globals()) {




    // Unify the element type of appending arrays.
    // Unify the element type of appending arrays.

    ArrayType *DAT = cast<ArrayType>(DGV->getValueType());
    ArrayType *DAT = cast<ArrayType>(DGV->getValueType());

    ArrayType *SAT = cast<ArrayType>(SGV.getValueType());
    ArrayType *SAT = cast<ArrayType>(SGV.getValueType());

    TypeMap.addTypeMapping(DAT->getElementType(), SAT->getElementType());
    TypeMap.addTypeMapping(DAT->getElementType(), SAT->getElementType());

  }
  }




  for (GlobalValue &SGV : *SrcM)
  for (GlobalValue &SGV : *SrcM)

    if (GlobalValue *DGV = getLinkedToGlobal(&SGV))
    if (GlobalValue *DGV = getLinkedToGlobal(&SGV)) {

      if (DGV->getType() == SGV.getType()) {

        // If the types of DGV and SGV are the same, it means that DGV is from

        // the source module and got added to DstM from a shared metadata.  We

        // shouldn't map this type to itself in case the type's components get

        // remapped to a new type from DstM (for instance, during the loop over

        // SrcM->getIdentifiedStructTypes() below).

        continue;

      }



      TypeMap.addTypeMapping(DGV->getType(), SGV.getType());
      TypeMap.addTypeMapping(DGV->getType(), SGV.getType());

    }




  for (GlobalValue &SGV : SrcM->aliases())
  for (GlobalValue &SGV : SrcM->aliases())

    if (GlobalValue *DGV = getLinkedToGlobal(&SGV))
    if (GlobalValue *DGV = getLinkedToGlobal(&SGV))

      TypeMap.addTypeMapping(DGV->getType(), SGV.getType());
      TypeMap.addTypeMapping(DGV->getType(), SGV.getType());




  // Incorporate types by name, scanning all the types in the source module.
  // Incorporate types by name, scanning all the types in the source module.

  // At this point, the destination module may have a type "%foo = { i32 }" for
  // At this point, the destination module may have a type "%foo = { i32 }" for

  // example.  When the source module got loaded into the same LLVMContext, if
  // example.  When the source module got loaded into the same LLVMContext, if

▲ Show 20 Lines • Show All 785 Lines • Show Last 20 Lines
llvm/trunk/test/LTO/X86/Inputs/type-mapping-bug3.ll
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"

target triple = "x86_64-unknown-linux-gnu"



; T2 is the non-opaque struct required to trigger the uniqued T2.0 and T3.0 to

; respectively T2 and T3 in the destination module.

%"T2" = type { %"T3"* }

%"T3" = type opaque



; Use/refer to T2 so it gets added as an IdentifiedStructType.  The debug

; reference to !6 is required to transitively load the metadata !5.

define void @a(%"T2") !dbg !6 {

  ret void

}



!llvm.module.flags = !{!0, !1}

!llvm.dbg.cu = !{!2}

!0 = !{i32 1, !"ThinLTO", i32 0}

!1 = !{i32 2, !"Debug Info Version", i32 3}

!2 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !3, retainedTypes: !4)

!3 = !DIFile(filename: "f2", directory: "")

!4 = !{!5}



; This DICompositeType refers to !5 in type-mapping-bug3.ll

!5 = !DICompositeType(tag: DW_TAG_structure_type, flags: DIFlagFwdDecl, identifier: "SHARED")



!6 = distinct !DISubprogram(unit: !2)

llvm/trunk/test/LTO/X86/type-mapping-bug3.ll
; RUN: opt -module-summary -o %t0.o %S/Inputs/type-mapping-bug3.ll

; RUN: opt -module-summary -o %t1.o %s

; RUN: llvm-lto2 run -o %t2 %t0.o %t1.o -r %t0.o,a,px -r %t1.o,b,px -r %t1.o,c,px -r %t1.o,d,

;

; Test for the issue described in https://bugs.llvm.org/show_bug.cgi?id=40312



target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"

target triple = "x86_64-unknown-linux-gnu"



; T2 is the non-opaque struct required to trigger the uniqued T2.0 and T3.0 to

; respectively T2 and T3 in the destination module.

%"T2" = type { %"T3"* }

%"T3" = type opaque



; Use/refer to T2 so it gets added as an IdentifiedStructType.

define void @c(%"T2") {

    unreachable

}



; The global declaration that causes the assertion when its type is mapped to

; itself incorrectly.

declare void @d(%"T3"*)



define void @b() {

entry:

  %f.addr = alloca %"T3"*load %"T3"*, %"T3"** %f.addr



  ; The call with the getCalledValue() vs getCalledFunction() mismatch.

  call void @d(%"T3"* %0)

  unreachable

}



!llvm.dbg.cu = !{!2}

!llvm.module.flags = !{!0, !1}

!0 = !{i32 1, !"ThinLTO", i32 0}

!1 = !{i32 2, !"Debug Info Version", i32 3}

!2 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !3, retainedTypes: !4)

!3 = !DIFile(filename: "f1", directory: "")

!4 = !{!5}



; This DICompositeType is referenced by !5 in Inputs/type-mapping-bug3.ll

; causing the function type in !7 to be added to its module.

!5 = !DICompositeType(tag: DW_TAG_structure_type, templateParams: !6, identifier: "SHARED")

!6 = !{!7}



; The reference to d and T3 that gets loaded into %t0.o

!7 = !DITemplateValueParameter(value: void (%"T3"*)* @d)