[PATCH] D29240: IR: Consider two DISubprograms to be odr-equal if they have the same template parameters.

[Peter Collingbourne via llvm-commits]

On Sat, Feb 4, 2017 at 8:22 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > On 2017-Feb-03, at 18:03, Duncan P. N. Exon Smith via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
> >
> >>
> >> On 2017-Feb-03, at 17:49, Duncan P. N. Exon Smith <dexonsmith at apple.com>
> wrote:
> >>
> >>>
> >>> On 2017-Feb-03, at 17:34, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> >>>
> >>>
> >>>> On 2017-Feb-03, at 15:55, Peter Collingbourne <peter at pcc.me.uk>
> wrote:
> >>>>
> >>>> On Fri, Feb 3, 2017 at 3:34 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> >>>> I spent some more time on this and I understand the failure.
> >>>>
> >>>> I'm really uncomfortable with the fix, though.  It seems partial --
> it fixes the particular crash you found, but couldn't this happen with
> other fields in DISubprogram?
>
> I spent a few hours looking more carefully at the testcase and digging
> into how it happens.  I'm feeling better about your fix as a short-term
> solution.  While it's papering over a more general problem, I'm convinced
> it's safer than the current state.  If you add a comment explaining why you
> need to compare TemplateParams -- to avoid incorrect collisions in
> mapMetadata when RF_MoveDistinctMDs and a ODR-DISubprogram ("m_fn2" in your
> testcase) has a non-ODR template parameter (i.e., a DICompositeType that
> does not have an identifier; "D" in your testcase) -- the code change LGTM
> (comments on the testcase below).
>
> It might be nice to add a FIXME to decouple the uniquing logic from the
> ODR logic.  I thought for a long while about how to clean this up and I
> didn't find a clear path.  Ideally, we'd have something similar to
> DICompositeType::buildODRType for DISubprogram and DIDerivedType, and put
> the logic in LLParser and MetadataLoader.  However: while we can count on
> the identifier field of DICompositeType being loaded early because it's an
> MDString, the same is *not* true of the inputs to DISubprogram and
> DIDerivedType; building them lazily would be difficult.  I also considered
> adding custom logic to MDNodeMapper (in mapMetadata) that recognizes the
> pattern, and fixes the operands.  This might better.
>
>
> I think the testcase can be reduced.  This seems sufficient:
> --
>   void sink(void *);
>
>   class A {
>   public:
>     template <typename> void m_fn2() { static int a; }
>     virtual void m_fn1();
>   };
>
>   void foo() {
>     class B : public A {
>     public:
>       B() { m_fn2<B>(); }
>     };
>     sink(new B);
>   }
> --
>
> Adrian, does the testcase look good to you?  (Besides minimizing it, I
> don't have other comments.)
>

Thanks, I've uploaded a new version of the patch with the reduced test case
and a FIXME.

Peter


>
> >>>> Maybe -- I guess that's what I was alluding to in my commit message.
> Do you think anything could break if I just remove the
> MDNodeSubsetEqualImpl specialization of DISubprogram?
> >>>
> >>> Yes.  I suspect we'll get a big regression (compile-time? memory?) in
> -flto=full, and possible a regression in -flto=thin (which is what I was
> targeting at the time).  IIRC, the point of this was to (a) reduce number
> of nodes and (b) cut down on ValueMapper traffic (ironically, the same
> thing that it breaks).
> >>
> >> (It's possible that this didn't affect -flto=full much, and the more
> recent changes to -flto=thin make this optimization unnecessary.)
> >
> > It looks like there's also a DWARF correctness issue if you back this
> out (although I have not confirmed).  r203982 added
> test/Linker/type-unique-odr-a.ll (which r266548 augments).  I think
> r266548 was necessary to prevent regressing that testcase when I took away
> the string-based pointers.
> >
> >>> Of course, correctness should trump performance, but I'm pretty
> concerned about turning it off blind.  I'm also still looking for other
> options...
> >>>
> >>>> Also, where does the ValueMapper run in this opt invocation?  Is that
> what -run-twice does?
> >>>>
> >>>> Yes, -run-twice causes us to clone the module, which uses
> ValueMapper. See http://llvm-cs.pcc.me.uk/tools/opt/opt.cpp#719
> >>>
> >>> Got it.
> >>>
> >>>> Thanks,
> >>>> --
> >>>> --
> >>>> Peter
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>


-- 
-- 
Peter
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170206/032e81d3/attachment-0001.html>