[PATCH] D66814: [IRMover] Don't map globals if their types are the same

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In D66814#1666457 <https://reviews.llvm.org/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.



================
Comment at: llvm/lib/Linker/IRMover.cpp:765
+        // is from the source module and got added to DstM from a module
+        // summary.  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
----------------
pirama wrote:
> tejohnson wrote:
> > pirama wrote:
> > > pirama wrote:
> > > > tejohnson wrote:
> > > > > 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.
> > > > 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.
> > > 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.
> > 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?
> I incorrectly used "module summary" in the patch.  The value is getting loaded via a shared metadata node (!5 in both test inputs)
In the test SGV and DGV are @d?


================
Comment at: llvm/test/LTO/X86/Inputs/type-mapping-bug3.ll:23
+
+; This DICompositeType refers to by !5 in type-mapping-bug3.ll
+!5 = !DICompositeType(tag: DW_TAG_structure_type, flags: DIFlagFwdDecl, identifier: "SHARED")
----------------
"refers to by" doesn't make sense. Do you mean "refers to !5 in..."?




================
Comment at: llvm/test/LTO/X86/type-mapping-bug3.ll:3
+; 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,px
+;
----------------
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?


================
Comment at: llvm/test/LTO/X86/type-mapping-bug3.ll:20
+
+; The global declaration that causes the assertion.
+declare void @d(%"T3"*)
----------------
"when its type is mapped to itself incorrectly"?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66814/new/

https://reviews.llvm.org/D66814