[llvm] r204107 - DwarfDebug: Only unique retained types instead of all types.

[Adrian Prantl]

On Mar 24, 2014, at 2:08 PM, Eric Christopher <echristo at gmail.com> wrote:

> On Mon, Mar 17, 2014 at 7:53 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> On Mon, Mar 17, 2014 at 7:35 PM, Adrian Prantl <aprantl at apple.com> wrote:
>>> Author: adrian
>>> Date: Mon Mar 17 21:35:03 2014
>>> New Revision: 204107
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=204107&view=rev
>>> Log:
>>> DwarfDebug: Only unique retained types instead of all types.
>>> This is a follow-up to r203983 based on feedback from dblaikie and mren (Thanks!)
>>> No functionality change.
>> 
>> Thanks!
>> 
>>> 
>>> Modified:
>>>    llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>>>    llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
>>> 
>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp?rev=204107&r1=204106&r2=204107&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp (original)
>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp Mon Mar 17 21:35:03 2014
>>> @@ -846,8 +846,13 @@ void DwarfDebug::beginModule() {
>>>     for (unsigned i = 0, e = EnumTypes.getNumElements(); i != e; ++i)
>>>       CU->getOrCreateTypeDIE(EnumTypes.getElement(i));
>>>     DIArray RetainedTypes = CUNode.getRetainedTypes();
>>> -    for (unsigned i = 0, e = RetainedTypes.getNumElements(); i != e; ++i)
>>> -      CU->getOrCreateTypeDIE(RetainedTypes.getElement(i));
>>> +    for (unsigned i = 0, e = RetainedTypes.getNumElements(); i != e; ++i) {
>>> +      DIType Ty(RetainedTypes.getElement(i));
>>> +      // The retained types array by design contains pointers to
>>> +      // MDNodes rather than DIRefs. Unique them here.
>>> +      DIType UniqueTy(resolve(Ty.getRef()));
>>> +      CU->getOrCreateTypeDIE(UniqueTy);
>> 
>> One thought I've had here: should we, rather than resolving the
>> references and emitting them, avoid emitting the type if the resolved
>> version isn't from this compilation unit?
>> 
>> I'm not sure there's a good reason here, but I'm just tossing it
>> around in my head - I wonder if we could get into confusing situations
>> (with types as you've described in other recent threads - where two
>> ODR-correct definitions, written in different files, may end up
>> emitted under the "wrong" compilation unit). I'm not sure if this
>> would actually be bad - but, for example, with my line table work it
>> might be a tiny bit more verbose to emit a type from the wrong
>> compilation unit since it wouldn't take advantage of the comp_dir as
>> much (if the type was actually in a header in that directory) or might
>> introduce an extra file name in the line table that would  otherwise
>> have been avoided.
>> 
>> This is ultra low priority - just sanity checking to see if maybe
>> there are issues with emitting the type from the wrong unit.
>> 
> 
> I think you're correct here. The way the code is written we're adding
> retained types as we go over the compile unit. We should probably do a
> check if the current compile unit is the same as the compile unit for
> the type that we're about to emit since the type merging would have
> put it in doubt which compile unit a retained type actually belonged
> to.
> 
>>> +    }
>>>     // Emit imported_modules last so that the relevant context is already
>>>     // available.
>>>     for (unsigned i = 0, e = ImportedEntities.getNumElements(); i != e; ++i)
>>> 
>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp?rev=204107&r1=204106&r2=204107&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp (original)
>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp Mon Mar 17 21:35:03 2014
>>> @@ -971,6 +971,8 @@ DIE *DwarfUnit::getOrCreateTypeDIE(const
>>> 
>>>   DIType Ty(TyNode);
>>>   assert(Ty.isType());
>>> +  assert(*&Ty == resolve(Ty.getRef()) &&
>> 
>> Why's the "*&Ty" required? That seems a bit strange/subtle...
>> 
> 
> Agreed. Adrian?

Absolutely. Removed in r204673.

-- adrian