<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Mar 7, 2016 at 9:00 AM, Mehdi Amini <span dir="ltr"><<a href="mailto:mehdi.amini@apple.com" target="_blank">mehdi.amini@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 style="word-wrap:break-word"><br><div><div><div class="h5"><blockquote type="cite"><div>On Mar 7, 2016, at 6:30 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Mar 7, 2016 at 1:45 AM, Mehdi Amini via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><br>
> On Mar 6, 2016, at 5:31 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank">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" target="_blank">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>
</div></div>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></blockquote><div><br></div><div>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... </div></div></div></div></div></blockquote><div><br></div></div></div><div>There's no correctness issue involved. The worst would be that more hashes would collide in the map and you need to probe longer.</div><div>(If this does not answer, I'm not sure I understood what you meant)</div></div></div></blockquote><div><br></div><div>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)?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><span class="HOEnZb"><font color="#888888"><div><br></div><div><br></div><div>-- </div><div>Mehdi</div></font></span><div><div class="h5"><div><br></div><div><br></div><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><font color="#888888"><br>
--<br>
Mehdi<br>
</font></span><div><div><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" target="_blank">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>
</div></div></blockquote></div><br></div></div>
</div></blockquote></div></div></div><br></div></blockquote></div><br></div></div>