<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Mar 7, 2016 at 12:34 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br>
> On 2016-Mar-07, at 11:49, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Mon, Mar 7, 2016 at 9:00 AM, Mehdi Amini <<a href="mailto:mehdi.amini@apple.com">mehdi.amini@apple.com</a>> wrote:<br>
><br>
>> On Mar 7, 2016, at 6:30 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>><br>
>><br>
>> On Mon, Mar 7, 2016 at 1:45 AM, Mehdi Amini via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> > On Mar 6, 2016, at 5:31 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
>> ><br>
>> > Sorry, I thought I'd responded back in January. This approach SGTM.<br>
>> ><br>
>> > I have some ideas about field choice below. If you've profiled -flto -g<br>
>> > with something non-trivial, then I'm happy for you to commit (LGTM!), but<br>
>> > you might be able to extract even better performance.<br>
>> ><br>
>> >> On 2016-Mar-05, at 23:13, Mehdi AMINI <<a href="mailto:mehdi.amini@apple.com">mehdi.amini@apple.com</a>> wrote:<br>
>> >><br>
>> >> joker.eph updated this revision to Diff 49892.<br>
>> >> joker.eph updated the summary for this revision.<br>
>> >> joker.eph added a comment.<br>
>> >><br>
>> >> Change the order of arguments as suggested by Eric.<br>
>> >><br>
>> >> Duncan?<br>
>> >><br>
>> >><br>
>> >> <a href="http://reviews.llvm.org/D16571" rel="noreferrer" target="_blank">http://reviews.llvm.org/D16571</a><br>
>> >><br>
>> >> Files:<br>
>> >> lib/IR/LLVMContextImpl.h<br>
>> >><br>
>> >> Index: lib/IR/LLVMContextImpl.h<br>
>> >> ===================================================================<br>
>> >> --- lib/IR/LLVMContextImpl.h<br>
>> >> +++ lib/IR/LLVMContextImpl.h<br>
>> >> @@ -365,8 +365,7 @@<br>
>> >> ExtraData == RHS->getRawExtraData();<br>
>> >> }<br>
>> >> unsigned getHashValue() const {<br>
>> >> - return hash_combine(Tag, Name, File, Line, Scope, BaseType, SizeInBits,<br>
>> >> - AlignInBits, OffsetInBits, Flags, ExtraData);<br>
>> >> + return hash_combine(Name, File, Line, Scope, BaseType);<br>
>> ><br>
>> > I suspect the most important distinguishers are:<br>
>> > - Tag<br>
>> > - Name<br>
>> > - BaseType<br>
>> > - Scope<br>
>> ><br>
>> > This tells you the kind of the derived type (pointer, typedef, etc.),<br>
>> > its name (if any), what it's derived from, and where it's defined. In<br>
>> > other words, I suggest dropping File/Line (which is often left blank)<br>
>> > and adding back Tag.<br>
>> ><br>
>> > However, you should confirm that dropping File/Line doesn't hurt<br>
>> > performance of -flto=full.<br>
>><br>
>> Unfortunately it does:<br>
>><br>
>> !317 = !DIDerivedType(tag: DW_TAG_typedef, name: "size_type", file: !234, line: 97, baseType: !268)<br>
>> !578 = !DIDerivedType(tag: DW_TAG_typedef, name: "size_type", file: !544, line: 1709, baseType: !268)<br>
>> !15757 = !DIDerivedType(tag: DW_TAG_typedef, name: "size_type", file: !15753, line: 36, baseType: !268)<br>
>> ....<br>
>><br>
>><br>
>> These are corresponding to declaration like:<br>
>><br>
>> typedef typename __alloc_traits::size_type size_type;<br>
>><br>
>> in classes like std::vector_base.<br>
>><br>
>> There are similar corner cases with the other types.<br>
>><br>
>> What's the behavior when you drop file/line from these?<br>
>><br>
>> We could probably manufacture correct C++ code that still tickles those problems:<br>
>><br>
>> foo.def<br>
>> typedef int x;<br>
>><br>
>> a.h:<br>
>> struct a {<br>
>> #include "foo.def"<br>
>> };<br>
>><br>
>> b.h:<br>
>> struct b {<br>
>> #include "bar.def"<br>
>> };<br>
>><br>
>> So maybe this is a bug we should fix regardless? But maybe that's difficult...<br>
><br>
> There's no correctness issue involved. The worst would be that more hashes would collide in the map and you need to probe longer.<br>
> (If this does not answer, I'm not sure I understood what you meant)<br>
><br>
> 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)?<br>
<br>
</div></div>This removes the file/line from DenseMapInfo::getHashValue, but leaves<br>
DenseMapInfo::getEqualTo unchanged. This should only impact compile<br>
time; in other words, there should be no semantic change.<br>
<br>
The tradeoff here is:<br>
- Fewer fields => the hash cheaper to calculate.<br>
- Fewer fields => more hash collisions => more probing.<br></blockquote><div><br></div><div>Ah, thanks for the context. Sorry for the confusion.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
><br>
><br>
><br>
> --<br>
> Mehdi<br>
><br>
><br>
><br>
>><br>
>><br>
>> --<br>
>> Mehdi<br>
>><br>
>><br>
>><br>
>><br>
>> ><br>
>> >> }<br>
>> >> };<br>
>> >><br>
>> >> @@ -422,9 +421,7 @@<br>
>> >> Identifier == RHS->getIdentifier();<br>
>> >> }<br>
>> >> unsigned getHashValue() const {<br>
>> >> - return hash_combine(Tag, Name, File, Line, Scope, BaseType, SizeInBits,<br>
>> >> - AlignInBits, OffsetInBits, Flags, Elements, RuntimeLang,<br>
>> >> - VTableHolder, TemplateParams, Identifier);<br>
>> >> + return hash_combine(Name, File, Line, Scope, SizeInBits, Elements);<br>
>> ><br>
>> > I feel like you can safely drop SizeInBits and Elements. I think it's<br>
>> > unlikely the other four fields will collide.<br>
>> ><br>
>> > Again, you should profile -flto=full with -g before dropping this.<br>
>> ><br>
>> >> }<br>
>> >> };<br>
>> >><br>
>> >> @@ -518,10 +515,7 @@<br>
>> >> Variables == RHS->getRawVariables();<br>
>> >> }<br>
>> >> unsigned getHashValue() const {<br>
>> >> - return hash_combine(Scope, Name, LinkageName, File, Line, Type,<br>
>> >> - IsLocalToUnit, IsDefinition, ScopeLine, ContainingType,<br>
>> >> - Virtuality, VirtualIndex, Flags, IsOptimized,<br>
>> >> - TemplateParams, Declaration, Variables);<br>
>> >> + return hash_combine(File, Line, Type, Scope);<br>
>> >> }<br>
>> >> };<br>
>> ><br>
>> > It feels strange to leave both Name and LinkageName out here. I'm not<br>
>> > sure it's wrong though. As long as you've profiled this.<br>
>> ><br>
>> >><br>
>> >><br>
>> >><br>
>> >> <D16571.49892.patch><br>
>><br>
>> _______________________________________________<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/mailman/listinfo/llvm-commits</a><br>
<br>
</div></div></blockquote></div><br></div></div>