[llvm-commits] [llvm] r94260 - in /llvm/trunk: lib/Transforms/Utils/PromoteMemoryToRegister.cpp test/Transforms/Mem2Reg/ConvertDebugInfo.ll

Victor Hernandez vhernandez at apple.com
Mon Jan 25 09:43:26 PST 2010


On Jan 24, 2010, at 11:39 PM, Chris Lattner wrote:

> 
> On Jan 22, 2010, at 4:17 PM, Victor Hernandez wrote:
> 
>> Author: hernande
>> Date: Fri Jan 22 18:17:34 2010
>> New Revision: 94260
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=94260&view=rev
>> Log:
>> In mem2reg, for all alloca/stores that get promoted where the alloca has an associated llvm.dbg.declare instrinsic, insert an llvm.dbg.var intrinsic before each store
> 
> Hi Victor,
> 
>> -    
>> +    void ConvertDebugDeclareToDebugValue(DbgDeclareInst *DDI, StoreInst* SI,
>> +                                         uint64_t Offset);
> 
> You're placing your *'s inconsistently.  Please format code like DDI, not like SI.
> 
>> 
>> +/// Finds the llvm.dbg.declare intrinsic corresponding to an alloca if any.
>> +static DbgDeclareInst *findDbgDeclare(AllocaInst *AI) {
>> +  Function *F = AI->getParent()->getParent();
>> +  for (Function::iterator FI = F->begin(), FE = F->end(); FI != FE; ++FI)
>> +    for (BasicBlock::iterator BI = (*FI).begin(), BE = (*FI).end();
>> +         BI != BE; ++BI)
>> +      if (DbgDeclareInst *DDI = dyn_cast<DbgDeclareInst>(BI))
>> +        if (DDI->getAddress() == AI)
>> +          return DDI;
> 
> This is *HIGHLY* inefficient.  Did you look at the impact of this on compile time?  I suspect that it will have a high hit particularly for stuff that doesn't use debug information.
> 
> A better solution is to add a MDNode method that queries to see if an mdnode exists.  Given that, you could write something like:
> 
>  // See if there is a function local MDNode with one argument that is the alloca.
>  if (MDNode *DebugNode = MDNode::getIfExists(Ctx, &AI, 1, true))
>    for (Value::use_iterator UI = DebugNode->use_begin(), E = DebugNode->use_end(); UI != E; ++UI)
>      if (DbgDeclareInst *DDI = dyn_cast<DbgDeclareInst>(BI))
>         ... found it ...
> 
> Also, you should only do this once per alloca, not once per store to the alloca.  Please revert this patch until the algorithm can be fixed.

I will revert it now.

I don't understand how MDNode::getIfExists() will be faster, because just to determine requires much work when there is not metadata.  I still need to iterate over all operands of all instructions (or at least the intrinsic calls) to determine if there is a function-local metadata hiding in there.  I am not aware of an existing more efficient means to do this, since metadata does not show up on use lists.

How about a function keeping a list of all its funciton-local metadata?  Is that too expensive?  Will it to be too easy to invalidate?


> -Chris
> 
> 
>> +
>> +  return 0;
>> +}
>> +
>> void PromoteMem2Reg::run() {
>>  Function &F = *DF.getRoot()->getParent();
>> 
>> @@ -344,6 +360,8 @@
>> 
>>      // Finally, after the scan, check to see if the store is all that is left.
>>      if (Info.UsingBlocks.empty()) {
>> +        // Record debuginfo for the store before removing it.
>> +        ConvertDebugDeclareToDebugValue(findDbgDeclare(AI), Info.OnlyStore, 0);
>>        // Remove the (now dead) store and alloca.
>>        Info.OnlyStore->eraseFromParent();
>>        LBI.deleteValue(Info.OnlyStore);
>> @@ -370,8 +388,11 @@
>>      if (Info.UsingBlocks.empty()) {
>> 
>>        // Remove the (now dead) stores and alloca.
>> +        DbgDeclareInst *DDI = findDbgDeclare(AI);
>>        while (!AI->use_empty()) {
>>          StoreInst *SI = cast<StoreInst>(AI->use_back());
>> +          // Record debuginfo for the store before removing it.
>> +          ConvertDebugDeclareToDebugValue(DDI, SI, 0);
>>          SI->eraseFromParent();
>>          LBI.deleteValue(SI);
>>        }
>> @@ -833,6 +854,18 @@
>>  }
>> }
>> 
>> +// Inserts a llvm.dbg.value instrinsic before the stores to an alloca'd value
>> +// that has an associated llvm.dbg.decl intrinsic.
>> +void PromoteMem2Reg::ConvertDebugDeclareToDebugValue(DbgDeclareInst *DDI,
>> +                                                     StoreInst* SI,
>> +                                                     uint64_t Offset) {
>> +  if (!DDI) return;
>> +
>> +  if (!DIF)
>> +    DIF = new DIFactory(*SI->getParent()->getParent()->getParent());
>> +  DIF->InsertDbgValueIntrinsic(SI->getOperand(0), Offset,
>> +                               DIVariable(DDI->getVariable()), SI);
>> +}
>> 
>> // QueuePhiNode - queues a phi-node to be added to a basic-block for a specific
>> // Alloca returns true if there wasn't already a phi-node for that variable
>> @@ -946,6 +979,8 @@
>> 
>>      // what value were we writing?
>>      IncomingVals[ai->second] = SI->getOperand(0);
>> +      // Record debuginfo for the store before removing it.
>> +      ConvertDebugDeclareToDebugValue(findDbgDeclare(Dest), SI, 0);
>>      BB->getInstList().erase(SI);
>>    }
>>  }
>> 
>> Added: llvm/trunk/test/Transforms/Mem2Reg/ConvertDebugInfo.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Mem2Reg/ConvertDebugInfo.ll?rev=94260&view=auto
>> 
>> ==============================================================================
>> --- llvm/trunk/test/Transforms/Mem2Reg/ConvertDebugInfo.ll (added)
>> +++ llvm/trunk/test/Transforms/Mem2Reg/ConvertDebugInfo.ll Fri Jan 22 18:17:34 2010
>> @@ -0,0 +1,31 @@
>> +; RUN: opt < %s -mem2reg -S | FileCheck %s
>> +
>> +target datalayout = "E-p:64:64:64-a0:0:8-f32:32:32-f64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-v64:64:64-v128:128:128"
>> +
>> +define double @testfunc(i32 %i, double %j) {
>> +	%I = alloca i32		; <i32*> [#uses=4]
>> +  call void @llvm.dbg.declare(metadata !{i32* %I}, metadata !0)
>> +	%J = alloca double		; <double*> [#uses=2]
>> +  call void @llvm.dbg.declare(metadata !{double* %J}, metadata !1)
>> +; CHECK: call void @llvm.dbg.value(metadata !{i32 %i}, i64 0, metadata !0)
>> +	store i32 %i, i32* %I
>> +; CHECK: call void @llvm.dbg.value(metadata !{double %j}, i64 0, metadata !1)
>> +	store double %j, double* %J
>> +	%t1 = load i32* %I		; <i32> [#uses=1]
>> +	%t2 = add i32 %t1, 1		; <i32> [#uses=1]
>> +	store i32 %t2, i32* %I
>> +	%t3 = load i32* %I		; <i32> [#uses=1]
>> +	%t4 = sitofp i32 %t3 to double		; <double> [#uses=1]
>> +	%t5 = load double* %J		; <double> [#uses=1]
>> +	%t6 = fmul double %t4, %t5		; <double> [#uses=1]
>> +	ret double %t6
>> +}
>> +
>> +declare void @llvm.dbg.declare(metadata, metadata) nounwind readnone
>> +
>> +!bar = !{!0}
>> +!foo = !{!2}
>> +
>> +!0 = metadata !{i32 459008, metadata !1, metadata !"foo", metadata !2, i32 5, metadata !"foo"} ; [ DW_TAG_auto_variable ]
>> +!1 = metadata !{i32 459008, metadata !1, metadata !"foo", metadata !0, i32 5, metadata !1} ; [ DW_TAG_auto_variable ]
>> +!2 = metadata !{i32 458804, i32 0, metadata !2, metadata !"foo", metadata !"bar", metadata !"bar", metadata !2, i32 3, metadata !0, i1 false, i1 true} ; [ DW_TAG_variable ]
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 





More information about the llvm-commits mailing list