[PATCH] D16571: Compute the DISubprogram hash key partially (NFC)

[David Blaikie via llvm-commits]

On Mon, Mar 7, 2016 at 9:00 AM, Mehdi Amini <mehdi.amini at apple.com> wrote:

>
> On Mar 7, 2016, at 6:30 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Mon, Mar 7, 2016 at 1:45 AM, Mehdi Amini via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>>
>> > On Mar 6, 2016, at 5:31 PM, Duncan P. N. Exon Smith <
>> dexonsmith at apple.com> wrote:
>> >
>> > Sorry, I thought I'd responded back in January.  This approach SGTM.
>> >
>> > I have some ideas about field choice below.  If you've profiled -flto -g
>> > with something non-trivial, then I'm happy for you to commit (LGTM!),
>> but
>> > you might be able to extract even better performance.
>> >
>> >> On 2016-Mar-05, at 23:13, Mehdi AMINI <mehdi.amini at apple.com> wrote:
>> >>
>> >> joker.eph updated this revision to Diff 49892.
>> >> joker.eph updated the summary for this revision.
>> >> joker.eph added a comment.
>> >>
>> >> Change the order of arguments as suggested by Eric.
>> >>
>> >> Duncan?
>> >>
>> >>
>> >> http://reviews.llvm.org/D16571
>> >>
>> >> Files:
>> >> lib/IR/LLVMContextImpl.h
>> >>
>> >> Index: lib/IR/LLVMContextImpl.h
>> >> ===================================================================
>> >> --- lib/IR/LLVMContextImpl.h
>> >> +++ lib/IR/LLVMContextImpl.h
>> >> @@ -365,8 +365,7 @@
>> >>           ExtraData == RHS->getRawExtraData();
>> >>  }
>> >>  unsigned getHashValue() const {
>> >> -    return hash_combine(Tag, Name, File, Line, Scope, BaseType,
>> SizeInBits,
>> >> -                        AlignInBits, OffsetInBits, Flags, ExtraData);
>> >> +    return hash_combine(Name, File, Line, Scope, BaseType);
>> >
>> > I suspect the most important distinguishers are:
>> > - Tag
>> > - Name
>> > - BaseType
>> > - Scope
>> >
>> > This tells you the kind of the derived type (pointer, typedef, etc.),
>> > its name (if any), what it's derived from, and where it's defined.  In
>> > other words, I suggest dropping File/Line (which is often left blank)
>> > and adding back Tag.
>> >
>> > However, you should confirm that dropping File/Line doesn't hurt
>> > performance of -flto=full.
>>
>> Unfortunately it does:
>>
>> !317 = !DIDerivedType(tag: DW_TAG_typedef, name: "size_type", file: !234,
>> line: 97, baseType: !268)
>> !578 = !DIDerivedType(tag: DW_TAG_typedef, name: "size_type", file: !544,
>> line: 1709, baseType: !268)
>> !15757 = !DIDerivedType(tag: DW_TAG_typedef, name: "size_type", file:
>> !15753, line: 36, baseType: !268)
>> ....
>>
>>
>> These are corresponding to declaration like:
>>
>>          typedef typename __alloc_traits::size_type       size_type;
>>
>> in classes like std::vector_base.
>>
>> There are similar corner cases with the other types.
>>
>
> What's the behavior when you drop file/line from these?
>
> We could probably manufacture correct C++ code that still tickles those
> problems:
>
> foo.def
> typedef int x;
>
> a.h:
> struct a {
>   #include "foo.def"
> };
>
> b.h:
> struct b {
>   #include "bar.def"
> };
>
> So maybe this is a bug we should fix regardless? But maybe that's
> difficult...
>
>
> There's no correctness issue involved. The worst would be that more hashes
> would collide in the map and you need to probe longer.
> (If this does not answer, I'm not sure I understood what you meant)
>

If we removed the file/line from those typedefs, wouldn't they collapse to
a single DI node during LTO linking? If that happened, I imagine we might
have trouble tracking them separately (eg: correctly describing variables
as being on one type but not the other, etc)?


>
>
> --
> Mehdi
>
>
>
>
>
>>
>> --
>> Mehdi
>>
>>
>>
>>
>> >
>> >>  }
>> >> };
>> >>
>> >> @@ -422,9 +421,7 @@
>> >>           Identifier == RHS->getIdentifier();
>> >>  }
>> >>  unsigned getHashValue() const {
>> >> -    return hash_combine(Tag, Name, File, Line, Scope, BaseType,
>> SizeInBits,
>> >> -                        AlignInBits, OffsetInBits, Flags, Elements,
>> RuntimeLang,
>> >> -                        VTableHolder, TemplateParams, Identifier);
>> >> +    return hash_combine(Name, File, Line, Scope, SizeInBits,
>> Elements);
>> >
>> > I feel like you can safely drop SizeInBits and Elements.  I think it's
>> > unlikely the other four fields will collide.
>> >
>> > Again, you should profile -flto=full with -g before dropping this.
>> >
>> >>  }
>> >> };
>> >>
>> >> @@ -518,10 +515,7 @@
>> >>           Variables == RHS->getRawVariables();
>> >>  }
>> >>  unsigned getHashValue() const {
>> >> -    return hash_combine(Scope, Name, LinkageName, File, Line, Type,
>> >> -                        IsLocalToUnit, IsDefinition, ScopeLine,
>> ContainingType,
>> >> -                        Virtuality, VirtualIndex, Flags, IsOptimized,
>> >> -                        TemplateParams, Declaration, Variables);
>> >> +    return hash_combine(File, Line, Type, Scope);
>> >>  }
>> >> };
>> >
>> > It feels strange to leave both Name and LinkageName out here.  I'm not
>> > sure it's wrong though.  As long as you've profiled this.
>> >
>> >>
>> >>
>> >>
>> >> <D16571.49892.patch>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160307/4d910dc7/attachment.html>