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

[Duncan P. N. Exon Smith via llvm-commits]

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

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