[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
llvm-commits at lists.llvm.org
Fri Feb 3 18:03:17 PST 2017
> 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?
>>> 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.
More information about the llvm-commits