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

[David Blaikie via llvm-commits]

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


>
> --
> 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/777b383b/attachment.html>