[llvm-commits] [llvm] r93531 - in /llvm/trunk: include/llvm/Analysis/DebugInfo.h include/llvm/IntrinsicInst.h include/llvm/Intrinsics.td lib/Analysis/DebugInfo.cpp lib/CodeGen/SelectionDAG/FastISel.cpp lib/VMCore/AutoUpgrade.cpp lib/VMCore/IntrinsicInst.cpp lib/VMCore/Verifier.cpp test/Assembler/functionlocal-metadata.ll test/DebugInfo/2009-10-16-Scope.ll test/DebugInfo/printdbginfo2.ll

[Chris Lattner]

On Jan 15, 2010, at 11:04 AM, Victor Hernandez wrote:

> Author: hernande
> Date: Fri Jan 15 13:04:09 2010
> New Revision: 93531
>
> URL: http://llvm.org/viewvc/llvm-project?rev=93531&view=rev
> Log:
> Improve llvm.dbg.declare intrinsic by referring directly to the  
> storage in its first argument, via function-local metadata (instead  
> of via a bitcast).
> This patch also cleans up code that expects there to be a bitcast in  
> the first argument and testcases that call llvm.dbg.declare.
> It also strips old llvm.dbg.declare intrinsics that did not pass  
> metadata as the first argument.

Great!

Please remove OnlyUsedByDbgInfoIntrinsics and its callers, since it is  
now dead with this change. Also InstCombiner::hasOneUsePlusDeclare and  
probably other stuff can be removed.

> +++ llvm/trunk/lib/Analysis/DebugInfo.cpp Fri Jan 15 13:04:09 2010
> @@ -1034,26 +1032,22 @@
> /// InsertDeclare - Insert a new llvm.dbg.declare intrinsic call.
> Instruction *DIFactory::InsertDeclare(Value *Storage, DIVariable D,
>                                       Instruction *InsertBefore) {
>   if (!DeclareFn)
>     DeclareFn = Intrinsic::getDeclaration(&M, Intrinsic::dbg_declare);
>
> +  Value *Elts[] = { Storage };
> +  Value *Args[] = { MDNode::get(Storage->getContext(), Elts, 1),  
> D.getNode() };

You can just pass in &Storage to eliminate the Elts array.

>   return CallInst::Create(DeclareFn, Args, Args+2, "", InsertBefore);
> }
>
> /// InsertDeclare - Insert a new llvm.dbg.declare intrinsic call.
> Instruction *DIFactory::InsertDeclare(Value *Storage, DIVariable D,
>                                       BasicBlock *InsertAtEnd) {
>   if (!DeclareFn)
>     DeclareFn = Intrinsic::getDeclaration(&M, Intrinsic::dbg_declare);
>
> +  Value *Elts[] = { Storage };
> +  Value *Args[] = { MDNode::get(Storage->getContext(), Elts, 1),  
> D.getNode() };
>   return CallInst::Create(DeclareFn, Args, Args+2, "", InsertAtEnd);

Likewise.
> @@ -1258,25 +1252,24 @@
>
> +const DbgDeclareInst *llvm::findDbgDeclare(const Value *V) {

This function needs comments, and should be static within the function  
since it is only used by getLocationInfo.  findDbgGlobalDeclare should  
similarly be local to the file.

The bigger issue is that findDbgDeclare is linear time with the size  
of a function (aka, really really slow).  Because it is O(n),  
DbgInfoPrinter is O(n^2).  findDbgGlobalDeclare is also O(n) in #  
global variables with debug information.

I think that the answer here is to remove getLocationInfo and replace  
it with a more efficient interface, what do you think?

> +++ llvm/trunk/lib/VMCore/Verifier.cpp Fri Jan 15 13:04:09 2010
> @@ -1590,9 +1590,10 @@
>   default:
>     break;
>   case Intrinsic::dbg_declare:  // llvm.dbg.declare
> -    if (Constant *C = dyn_cast<Constant>(CI.getOperand(1)))
> -      Assert1(C && !isa<ConstantPointerNull>(C),
> -              "invalid llvm.dbg.declare intrinsic call", &CI);
> +    if (MDNode *MD = dyn_cast<MDNode>(CI.getOperand(1)))

this should use cast_or_null.

> +      if (Constant *C = dyn_cast<Constant>(MD->getOperand(0)))
> +        Assert1(C && !isa<ConstantPointerNull>(C),
> +                "invalid llvm.dbg.declare intrinsic call", &CI);

This assert is not checking for C being null correctly, nor is it  
checking for Op#1 not being MD correctly.

-Chris