<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Feb 13, 2018 at 7:00 AM, Rafael Avila de Espindola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Why do you want this?<br>
<br>
The name of LLVM types should not be important. The cases that we<br>
already have are a hack and I think the agreed direction is to have a<br>
single pointer type. That would make the type graph a DAG which is<br>
trivial to merge.<br></blockquote><div><br></div><div>Hi Rafael,</div><div><br></div><div>See his explanation in the reply on <a href="https://reviews.llvm.org/D43077">https://reviews.llvm.org/D43077</a> from early yesterday. It is leaving bitcasts that are blocking optimization.</div><div><br></div><div>Teresa</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Cheers,<br>
Rafael<br>
<br>
Eugene Leviant via Phabricator via llvm-commits<br>
<div><div class="gmail-h5"><<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> writes:<br>
<br>
> evgeny777 created this revision.<br>
> evgeny777 added reviewers: mehdi_amini, tejohnson.<br>
> Herald added a subscriber: eraman.<br>
><br>
> This patch was separated from <a href="https://reviews.llvm.org/D43077" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D43077</a> after discussion with @tejohnson. It implements name based structure type mapping to<br>
> avoid mapping to wrong type which is isomorphic to one we're about to import.<br>
><br>
> Not sure whom to add for review.<br>
><br>
><br>
> <a href="https://reviews.llvm.org/D43199" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D43199</a><br>
><br>
> Files:<br>
> include/llvm/Linker/IRMover.h<br>
> lib/Linker/IRMover.cpp<br>
> test/ThinLTO/X86/Inputs/<wbr>struct-mapping-baz.ll<br>
> test/ThinLTO/X86/Inputs/<wbr>struct-mapping-foo.ll<br>
> test/ThinLTO/X86/struct-<wbr>mapping.ll<br>
><br>
</div></div>> Index: test/ThinLTO/X86/struct-<wbr>mapping.ll<br>
> ==============================<wbr>==============================<wbr>=======<br>
> --- test/ThinLTO/X86/struct-<wbr>mapping.ll<br>
> +++ test/ThinLTO/X86/struct-<wbr>mapping.ll<br>
> @@ -0,0 +1,46 @@<br>
> +; RUN: opt -module-summary %s -o %t1.bc<br>
> +<br>
> +; The output file name does matter, because it affetcs the function<br>
> +; import order. This is the reason struct-mapping-foo.ll becomes %t3<br>
> +; RUN: opt -module-summary %p/Inputs/struct-mapping-foo.<wbr>ll -o %t3.bc<br>
> +; RUN: opt -module-summary %p/Inputs/struct-mapping-baz.<wbr>ll -o %t2.bc<br>
> +<br>
> +; RUN: llvm-lto -thinlto-action=thinlink %t1.bc %t2.bc %t3.bc -o %t4.index.bc<br>
> +; RUN: llvm-lto -thinlto-action=import %t1.bc %t2.bc %t3.bc -thinlto-index=%t4.index.bc<br>
> +; RUN: llvm-dis %t1.bc.thinlto.imported.bc -o - | FileCheck --check-prefix=IMPORT %s<br>
> +<br>
> +; Here we check that new type mapping algorithm correctly mapped type of internal<br>
> +; member of struct.Baz to struct.Foo. Without it we'd map that type to struct.Bar, because<br>
> +; it is recursively isomorphic to struct.Foo and is defined first in source file.<br>
> +; IMPORT: %struct.Baz = type { i64, i64, %struct.Foo }<br>
> +<br>
> +target datalayout = "e-m:e-i64:64-f80:128-n8:16:<wbr>32:64-S128"<br>
> +target triple = "x86_64-pc-linux-gnu"<br>
> +<br>
> +%struct.Bar = type { i64, i64 }<br>
> +%struct.S = type { %struct.Foo* }<br>
> +%struct.Foo = type { i64, i64 }<br>
> +<br>
> +@B0 = local_unnamed_addr global %struct.Bar zeroinitializer, align 8<br>
> +@instance = local_unnamed_addr global %struct.S zeroinitializer, align 8<br>
> +@llvm.global_ctors = appending global [1 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 65535, void ()* @_GLOBAL__sub_I_main.cpp, i8* null }]<br>
> +<br>
> +; Function Attrs: norecurse nounwind readonly uwtable<br>
> +define i32 @main() local_unnamed_addr {<br>
> +entry:<br>
> + %0 = load %struct.Foo*, %struct.Foo** getelementptr inbounds (%struct.S, %struct.S* @instance, i64 0, i32 0), align 8<br>
> + %a = getelementptr inbounds %struct.Foo, %struct.Foo* %0, i64 0, i32 0<br>
> + %1 = load i64, i64* %a, align 8<br>
> + %conv = trunc i64 %1 to i32<br>
> + ret i32 %conv<br>
> +}<br>
> +<br>
> +declare %struct.Foo* @_Z6getFoov() local_unnamed_addr<br>
> +<br>
> +; Function Attrs: uwtable<br>
> +define internal void @_GLOBAL__sub_I_main.cpp() section ".text.startup" {<br>
> +entry:<br>
> + %call.i.i = tail call %struct.Foo* @_Z6getFoov()<br>
> + store %struct.Foo* %call.i.i, %struct.Foo** getelementptr inbounds (%struct.S, %struct.S* @instance, i64 0, i32 0), align 8<br>
> + ret void<br>
> +}<br>
> Index: test/ThinLTO/X86/Inputs/<wbr>struct-mapping-foo.ll<br>
> ==============================<wbr>==============================<wbr>=======<br>
> --- test/ThinLTO/X86/Inputs/<wbr>struct-mapping-foo.ll<br>
> +++ test/ThinLTO/X86/Inputs/<wbr>struct-mapping-foo.ll<br>
> @@ -0,0 +1,26 @@<br>
> +target datalayout = "e-m:e-i64:64-f80:128-n8:16:<wbr>32:64-S128"<br>
> +target triple = "x86_64-pc-linux-gnu"<br>
> +<br>
> +%struct.Foo = type { i64, i64 }<br>
> +%struct.Baz = type { i64, i64, %struct.Foo }<br>
> +<br>
> +@gFoo = local_unnamed_addr global %struct.Foo* null, align 8<br>
> +<br>
> +; Function Attrs: uwtable<br>
> +define %struct.Foo* @_Z6getFoov() local_unnamed_addr {<br>
> +entry:<br>
> + %0 = load %struct.Foo*, %struct.Foo** @gFoo, align 8<br>
> + %tobool = icmp eq %struct.Foo* %0, null<br>
> + br i1 %tobool, label %cond.false, label %cond.end<br>
> +<br>
> +cond.false: ; preds = %entry<br>
> + %call = tail call %struct.Baz* @_Z6getBazv()<br>
> + %f = getelementptr inbounds %struct.Baz, %struct.Baz* %call, i64 0, i32 2<br>
> + br label %cond.end<br>
> +<br>
> +cond.end: ; preds = %entry, %cond.false<br>
> + %cond = phi %struct.Foo* [ %f, %cond.false ], [ %0, %entry ]<br>
> + ret %struct.Foo* %cond<br>
> +}<br>
> +<br>
> +declare %struct.Baz* @_Z6getBazv() local_unnamed_addr<br>
> Index: test/ThinLTO/X86/Inputs/<wbr>struct-mapping-baz.ll<br>
> ==============================<wbr>==============================<wbr>=======<br>
> --- test/ThinLTO/X86/Inputs/<wbr>struct-mapping-baz.ll<br>
> +++ test/ThinLTO/X86/Inputs/<wbr>struct-mapping-baz.ll<br>
> @@ -0,0 +1,14 @@<br>
> +target datalayout = "e-m:e-i64:64-f80:128-n8:16:<wbr>32:64-S128"<br>
> +target triple = "x86_64-pc-linux-gnu"<br>
> +<br>
> +%struct.Baz = type { i64, i64, %struct.Foo }<br>
> +%struct.Foo = type { i64, i64 }<br>
> +<br>
> +@gBaz = local_unnamed_addr global %struct.Baz* null, align 8<br>
> +<br>
> +; Function Attrs: norecurse nounwind readonly uwtable<br>
> +define %struct.Baz* @_Z6getBazv() local_unnamed_addr {<br>
> +entry:<br>
> + %0 = load %struct.Baz*, %struct.Baz** @gBaz, align 8<br>
> + ret %struct.Baz* %0<br>
> +}<br>
> Index: lib/Linker/IRMover.cpp<br>
> ==============================<wbr>==============================<wbr>=======<br>
> --- lib/Linker/IRMover.cpp<br>
> +++ lib/Linker/IRMover.cpp<br>
> @@ -319,7 +319,7 @@<br>
> }<br>
><br>
> if (StructType *OldT =<br>
> - DstStructTypesSet.<wbr>findNonOpaque(ElementTypes, IsPacked)) {<br>
> + DstStructTypesSet.<wbr>findNonOpaque(ElementTypes, IsPacked, STy->getName())) {<br>
> STy->setName("");<br>
> return *Entry = OldT;<br>
> }<br>
> @@ -682,6 +682,14 @@<br>
> return NewGV;<br>
> }<br>
><br>
> +static StringRef getTypeNamePrefix(StringRef Name) {<br>
> + size_t DotPos = Name.rfind('.');<br>
> + return (DotPos == 0 || DotPos == StringRef::npos || Name.back() == '.' ||<br>
> + !isdigit(static_cast<unsigned char>(Name[DotPos + 1])))<br>
> + ? Name<br>
> + : Name.substr(0, DotPos);<br>
> +}<br>
> +<br>
> /// Loop over all of the linked values to compute type mappings. For example,<br>
> /// if we link "extern Foo *x" and "Foo *x = NULL", then we have two struct<br>
> /// types 'Foo' but one got renamed when the module was loaded into the same<br>
> @@ -729,14 +737,12 @@<br>
> }<br>
><br>
> // Check to see if there is a dot in the name followed by a digit.<br>
> - size_t DotPos = ST->getName().rfind('.');<br>
> - if (DotPos == 0 || DotPos == StringRef::npos ||<br>
> - ST->getName().back() == '.' ||<br>
> - !isdigit(static_cast<unsigned char>(ST->getName()[DotPos + 1])))<br>
> + auto STTypePrefix = getTypeNamePrefix(ST->getName(<wbr>));<br>
> + if (STTypePrefix.size()== ST->getName().size())<br>
> continue;<br>
><br>
> // Check to see if the destination module has a struct with the prefix name.<br>
> - StructType *DST = DstM.getTypeByName(ST-><wbr>getName().substr(0, DotPos));<br>
> + StructType *DST = DstM.getTypeByName(<wbr>STTypePrefix);<br>
> if (!DST)<br>
> continue;<br>
><br>
> @@ -901,7 +907,6 @@<br>
> Expected<Constant *> IRLinker::<wbr>linkGlobalValueProto(<wbr>GlobalValue *SGV,<br>
> bool ForAlias) {<br>
> GlobalValue *DGV = getLinkedToGlobal(SGV);<br>
> -<br>
> bool ShouldLink = shouldLink(DGV, *SGV);<br>
><br>
> // just missing from map<br>
> @@ -1409,12 +1414,14 @@<br>
><br>
> void IRMover::<wbr>IdentifiedStructTypeSet::<wbr>addNonOpaque(StructType *Ty) {<br>
> assert(!Ty->isOpaque());<br>
> + if (Ty->hasName())<br>
> + NonOpaqueStructNameMap.insert(<wbr>{getTypeNamePrefix(Ty-><wbr>getName()), Ty});<br>
> +<br>
> NonOpaqueStructTypes.insert(<wbr>Ty);<br>
> }<br>
><br>
> void IRMover::<wbr>IdentifiedStructTypeSet::<wbr>switchToNonOpaque(StructType *Ty) {<br>
> - assert(!Ty->isOpaque());<br>
> - NonOpaqueStructTypes.insert(<wbr>Ty);<br>
> + addNonOpaque(Ty);<br>
> bool Removed = OpaqueStructTypes.erase(Ty);<br>
> (void)Removed;<br>
> assert(Removed);<br>
> @@ -1427,10 +1434,16 @@<br>
><br>
> StructType *<br>
> IRMover::<wbr>IdentifiedStructTypeSet::<wbr>findNonOpaque(ArrayRef<Type *> ETypes,<br>
> - bool IsPacked) {<br>
> + bool IsPacked, StringRef Name) {<br>
> IRMover::StructTypeKeyInfo::<wbr>KeyTy Key(ETypes, IsPacked);<br>
> auto I = NonOpaqueStructTypes.find_as(<wbr>Key);<br>
> - return I == NonOpaqueStructTypes.end() ? nullptr : *I;<br>
> + if (I == NonOpaqueStructTypes.end())<br>
> + return nullptr;<br>
> + auto NI = NonOpaqueStructNameMap.find(<wbr>getTypeNamePrefix(Name));<br>
> + if (NI != NonOpaqueStructNameMap.end() &&<br>
> + IRMover::StructTypeKeyInfo::<wbr>KeyTy((*NI).second) == Key)<br>
> + return (*NI).second;<br>
> + return *I;<br>
> }<br>
><br>
> bool IRMover::<wbr>IdentifiedStructTypeSet::<wbr>hasType(StructType *Ty) {<br>
> Index: include/llvm/Linker/IRMover.h<br>
> ==============================<wbr>==============================<wbr>=======<br>
> --- include/llvm/Linker/IRMover.h<br>
> +++ include/llvm/Linker/IRMover.h<br>
> @@ -52,11 +52,16 @@<br>
> // The set of identified but non opaque structures in the composite module.<br>
> DenseSet<StructType *, StructTypeKeyInfo> NonOpaqueStructTypes;<br>
><br>
> + // Map between structure type name and instance. Used in findNonOpaque<br>
> + // to correctly map imported global variable type during ThinLTO import<br>
> + // phase.<br>
> + DenseMap<StringRef, StructType *> NonOpaqueStructNameMap;<br>
> public:<br>
> void addNonOpaque(StructType *Ty);<br>
> void switchToNonOpaque(StructType *Ty);<br>
> void addOpaque(StructType *Ty);<br>
> - StructType *findNonOpaque(ArrayRef<Type *> ETypes, bool IsPacked);<br>
> + StructType *findNonOpaque(ArrayRef<Type *> ETypes, bool IsPacked,<br>
> + StringRef Name);<br>
> bool hasType(StructType *Ty);<br>
> };<br>
><br>
> ______________________________<wbr>_________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature"><span style="font-family:Times;font-size:medium"><table cellspacing="0" cellpadding="0"><tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small"><td nowrap style="border-top:2px solid rgb(213,15,37)">Teresa Johnson |</td><td nowrap style="border-top:2px solid rgb(51,105,232)"> Software Engineer |</td><td nowrap style="border-top:2px solid rgb(0,153,57)"> <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> |</td><td nowrap style="border-top:2px solid rgb(238,178,17)"> 408-460-2413</td></tr></tbody></table></span></div>
</div></div>