[llvm] r204162 - Debug info: Remove OdrMemberMap from DwarfDebug, it's not necessary.

[David Blaikie]

On Fri, Mar 21, 2014 at 10:52 AM, Adrian Prantl <aprantl at apple.com> wrote:
>
> On Mar 21, 2014, at 10:44 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>> On Fri, Mar 21, 2014 at 10:34 AM, Manman Ren <manman.ren at gmail.com> wrote:
>>>
>>>
>>>
>>> On Tue, Mar 18, 2014 at 10:41 AM, Adrian Prantl <aprantl at apple.com> wrote:
>>>>
>>>> Author: adrian
>>>> Date: Tue Mar 18 12:41:15 2014
>>>> New Revision: 204162
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=204162&view=rev
>>>> Log:
>>>> Debug info: Remove OdrMemberMap from DwarfDebug, it's not necessary.
>>>> Follow-up to r203982.
>>>>
>>>> Modified:
>>>>    llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>>>>    llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h
>>>>    llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
>>>>    llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.h
>>>>
>>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp?rev=204162&r1=204161&r2=204162&view=diff
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp (original)
>>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp Tue Mar 18 12:41:15
>>>> 2014
>>>> @@ -369,7 +369,6 @@ bool DwarfDebug::isSubprogramContext(con
>>>> // scope then create and insert DIEs for these variables.
>>>> DIE *DwarfDebug::updateSubprogramScopeDIE(DwarfCompileUnit *SPCU,
>>>>                                           DISubprogram SP) {
>>>> -  SP = SPCU->getOdrUniqueSubprogram(resolve(SP.getContext()), SP);
>>>>   DIE *SPDie = SPCU->getDIE(SP);
>>>>
>>>>   assert(SPDie && "Unable to find subprogram DIE!");
>>>> @@ -604,7 +603,8 @@ DIE *DwarfDebug::constructScopeDIE(Dwarf
>>>>   if (!Scope || !Scope->getScopeNode())
>>>>     return NULL;
>>>>
>>>> -  DIScope DS(Scope->getScopeNode());
>>>> +  // Unique scope where applicable.
>>>> +  DIScope DS(resolve(DIScope(Scope->getScopeNode()).getRef()));
>>>
>>>
>>> Just to recapture what Adrian and I discussed earlier. We try not to unique
>>> types in the backend. All references to
>>> DITypes with an identifier should be converted to DIRef if possible (with
>>> the exception of the retained type list).
>>
>> OK - so I'm a bit confused by this code, then. Are you suggesting that
>> "we try not to unique types in the backend, but this case above is an
>> exception that we need to do in the backend for <reason>" or are you
>> saying that this code isn't an instance of what you would call
>> "uniquing types in the backend".
>>
>> It looks as if we still want to change this code so it doesn't loop
>> back through "uniquing" by getting a ref to the current entity and
>> "uniquing" it. Instead we should change getScopeNode to return an
>> MDNode or MDString as other "refs" do - and resolve it directly
>> (rather than having it reference the MDNode directly, querying that
>> for the name, then looking the name up to find (possibly the same)
>> MDNode)?
>>
>>>
>>>
>>> Adrian tried reverting r203982, the testing case for r203982 will still
>>> work, it is working because of his earlier commit on converting DIVariable's
>>> type to use DIRef.
>>>
>>>>
>>>>
>>>>   SmallVector<DIE *, 8> Children;
>>>>   DIE *ObjectPointer = NULL;
>>>>
>>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h?rev=204162&r1=204161&r2=204162&view=diff
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h (original)
>>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h Tue Mar 18 12:41:15
>>>> 2014
>>>> @@ -346,9 +346,6 @@ class DwarfDebug : public AsmPrinterHand
>>>>   /// of in DwarfCompileUnit.
>>>>   DenseMap<const MDNode *, DIE *> MDTypeNodeToDieMap;
>>>>
>>>> -  // Used to unique C++ member function declarations.
>>>> -  StringMap<const MDNode *> OdrMemberMap;
>>>> -
>>>>   // List of all labels used in aranges generation.
>>>>   std::vector<SymbolCU> ArangeLabels;
>>>>
>>>> @@ -700,11 +697,6 @@ public:
>>>>     return MDTypeNodeToDieMap.lookup(TypeMD);
>>>>   }
>>>>
>>>> -  /// \brief Look up or create an entry in the OdrMemberMap.
>>>> -  const MDNode *&getOrCreateOdrMember(StringRef Key) {
>>>> -    return OdrMemberMap.GetOrCreateValue(Key).getValue();
>>>> -  }
>>>> -
>>>>   /// \brief Emit all Dwarf sections that should come prior to the
>>>>   /// content.
>>>>   void beginModule();
>>>>
>>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp?rev=204162&r1=204161&r2=204162&view=diff
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp (original)
>>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp Tue Mar 18 12:41:15
>>>> 2014
>>>> @@ -1427,24 +1427,6 @@ DIE *DwarfUnit::getOrCreateNameSpace(DIN
>>>>   return NDie;
>>>> }
>>>>
>>>> -/// Unique C++ member function declarations based on their
>>>> -/// context and mangled name.
>>>> -DISubprogram
>>>> -DwarfUnit::getOdrUniqueSubprogram(DIScope Context, DISubprogram SP) const
>>>> {
>>>> -  if (!hasODR() ||
>>>> -      !Context.isCompositeType() ||
>>>> -      SP.getLinkageName().empty() ||
>>>> -      SP.isDefinition())
>>>> -    return SP;
>>>> -  // Create a key with the UID of the parent class and this SP's name.
>>>> -  Twine Key = SP.getContext().getName() + SP.getLinkageName();
>>>> -  const MDNode *&Entry = DD->getOrCreateOdrMember(Key.str());
>>>> -  if (!Entry)
>>>> -    Entry = &*SP;
>>>> -
>>>> -  return DISubprogram(Entry);
>>>> -}
>>>> -
>>>> /// getOrCreateSubprogramDIE - Create new DIE using SP.
>>>> DIE *DwarfUnit::getOrCreateSubprogramDIE(DISubprogram SP) {
>>>>   // Construct the context before querying for the existence of the DIE
>>>> in case
>>>> @@ -1452,8 +1434,10 @@ DIE *DwarfUnit::getOrCreateSubprogramDIE
>>>>   // declarations).
>>>>   DIScope Context = resolve(SP.getContext());
>>>>   DIE *ContextDIE = getOrCreateContextDIE(Context);
>>>> +
>>>>   // Unique declarations based on the ODR, where applicable.
>>>> -  SP = getOdrUniqueSubprogram(Context, SP);
>>>> +  SP = DISubprogram(DD->resolve(SP.getRef()));
>>>
>>>
>>> This line of code does nothing, since we don't have identifiers for
>>> DISubprogram.
>>
>> Nice catch - yeah... this is more of that loop-back "uniquing" anyway
>> - it'd be nice to get rid of any code like this in favor of fixing the
>> ways objects are referenced. (I understand there's a reasonable
>> exception when visiting the retained types list, potentially - though
>> we could think about that a bit further too)
>
> .. the exception being that we do need to store direct references to the types *somewhere*, otherwise they will be dropped.

Right - sorry, didn't mean we shouldn't store MDNode references to
types in retained types - that's clearly needed. I just meant that we
might do something different when we emit types from the retained
types list. (maybe we should build the ID (mangled name) -> type
mapping, then just emit the types by walking that map - then we
wouldn't have to do the uniquing resolve step - we would've already
done that work when we build up the resolver map)

>
>>
>> So we should remove this line of code, right?
>
> I'll take care of that,
> thanks again for noticing, Manman!
>
> -- adrian
>