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

[Mehdi Amini via llvm-commits]

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

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