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

Mon Mar 7 12:35:18 PST 2016

On Mon, Mar 7, 2016 at 12:34 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

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

Ah, thanks for the context. Sorry for the confusion.

>
> >
> >
> >
> > --
> > 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160307/a06b7b6a/attachment.html>