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

[David Blaikie]

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.

> +    }
>      // 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...

> +         "type was not uniqued, possible ODR violation.");
>
>    // Construct the context before querying for the existence of the DIE in case
>    // such construction creates the DIE.
> @@ -978,9 +980,6 @@ DIE *DwarfUnit::getOrCreateTypeDIE(const
>    DIE *ContextDIE = getOrCreateContextDIE(Context);
>    assert(ContextDIE);
>
> -  // Unique the type. This is a noop if the type has no unique identifier.
> -  Ty = DIType(resolve(Ty.getRef()));
> -
>    DIE *TyDIE = getDIE(Ty);
>    if (TyDIE)
>      return TyDIE;
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits