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

[Adrian Prantl]

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.

> 
> So we should remove this line of code, right?

I’ll take care of that,
thanks again for noticing, Manman!

-- adrian