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

[Duncan P. N. Exon Smith via llvm-commits]

> On 2016-Mar-07, at 11:49, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> 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)?

This removes the file/line from DenseMapInfo::getHashValue, but leaves
DenseMapInfo::getEqualTo unchanged.  This should only impact compile
time; in other words, there should be no semantic change.

The tradeoff here is:
- Fewer fields => the hash cheaper to calculate.
- Fewer fields => more hash collisions => more probing.

>  
> 
> 
> -- 
> 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