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

[David Blaikie]

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)

So we should remove this line of code, right?

- David

>
> Thanks,
> Manman
>
>>
>> +  assert(SP.Verify());
>>
>>    DIE *SPDie = getDIE(SP);
>>    if (SPDie)
>>
>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.h?rev=204162&r1=204161&r2=204162&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.h (original)
>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.h Tue Mar 18 12:41:15 2014
>> @@ -487,28 +487,6 @@ public:
>>
>>    virtual DwarfCompileUnit &getCU() = 0;
>>
>> -  /// \brief Return whether this compilation unit has the
>> -  /// one-definition-rule (ODR).  In C++ this allows the compiler to
>> -  /// perform type unique during LTO.
>> -  bool hasODR() const {
>> -    switch (getLanguage()) {
>> -    case dwarf::DW_LANG_C_plus_plus:
>> -    case dwarf::DW_LANG_C_plus_plus_03:
>> -    case dwarf::DW_LANG_C_plus_plus_11:
>> -      // For all we care, the C++ part of the language has the ODR and
>> -      // ObjC methods are not represented in a way that they could be
>> -      // confused with C++ member functions.
>> -    case dwarf::DW_LANG_ObjC_plus_plus:
>> -      return true;
>> -    default:
>> -      return false;
>> -    }
>> -  }
>> -
>> -  /// \brief Unique C++ member function declarations based on their
>> -  /// context+mangled name.
>> -  DISubprogram getOdrUniqueSubprogram(DIScope Context, DISubprogram SP)
>> const;
>> -
>>  protected:
>>    /// getOrCreateStaticMemberDIE - Create new static data member DIE.
>>    DIE *getOrCreateStaticMemberDIE(DIDerivedType DT);
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>