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

[Mehdi Amini via llvm-commits]

> 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 <mailto:llvm-commits at lists.llvm.org>> wrote:
> 
> > On Mar 6, 2016, at 5:31 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com <mailto: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 <mailto: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 <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)


-- 
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 <mailto:llvm-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <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/5c23ea85/attachment.html>