[cfe-commits] r110660 - in /cfe/trunk: lib/CodeGen/CGDebugInfo.cpp lib/CodeGen/CGDebugInfo.h lib/CodeGen/CGExprScalar.cpp lib/CodeGen/CodeGenFunction.cpp lib/CodeGen/CodeGenFunction.h test/CodeGen/2010-08-10-DbgConstant.c

[Devang Patel]

On Aug 10, 2010, at 8:25 AM, Chris Lattner wrote:

> On Aug 10, 2010, at 12:24 AM, Devang Patel wrote:
>> Author: dpatel
>> Date: Tue Aug 10 02:24:25 2010
>> New Revision: 110660
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=110660&view=rev
>> Log:
>> Even if a constant's evaluated value is used, emit debug info for the constant variable.
> 
> Ok, a couple of comments.
> 
>> +++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Tue Aug 10 02:24:25 2010
>> @@ -1801,6 +1801,17 @@
>>                                    true/*definition*/, Var);
>> }
>> 
>> +void CGDebugInfo::EmitGlobalVariable(llvm::Constant *C, const ValueDecl *VD,
>> +                                     CGBuilderTy &Builder) {
> 
> VD should come first, because it is the global variable you're emitting the debug info for, the constant is secondary.  Also, C should be named "Initializer" and/or you should add a doxygen comment explaining what this does.

Done.

> 
>> +++ cfe/trunk/lib/CodeGen/CGDebugInfo.h Tue Aug 10 02:24:25 2010
>> @@ -181,6 +181,10 @@
>>  /// EmitGlobalVariable - Emit information about an objective-c interface.
>>  void EmitGlobalVariable(llvm::GlobalVariable *GV, ObjCInterfaceDecl *Decl);
>> 
>> +  /// EmitGlobalVariable - Emit information about a constant.
>> +  void EmitGlobalVariable(llvm::Constant *C, const ValueDecl *VD, 
>> +                          CGBuilderTy &Builder);
> 
> This comment seems contradictory.  Are you emitting debug info for a global variable or a constant?

fixed.

> 
>> +++ cfe/trunk/lib/CodeGen/CGExprScalar.cpp Tue Aug 10 02:24:25 2010
>> @@ -149,6 +149,7 @@
>>    Expr::EvalResult Result;
>>    if (E->Evaluate(Result, CGF.getContext()) && Result.Val.isInt()) {
>>      assert(!Result.HasSideEffects && "Constant declref with side-effect?!");
>> +      CGF.EmitDeclRefExprDbgValue(E, Result.Val);
>>      return llvm::ConstantInt::get(VMContext, Result.Val.getInt());
> 
> This is the only caller of CGF.EmitDeclRefExprDbgValue.  Please pass the created ConstantInt to it instead of passing the APValue.

ok. done.
> 
>> +void CodeGenFunction::EmitDeclRefExprDbgValue(const DeclRefExpr *E, 
>> +                                              const APValue &AV) {
>> +  CGDebugInfo *Dbg = getDebugInfo();
>> +  if (!Dbg) return;
>> +
>> +  llvm::Constant *C = NULL;
>> +  if (AV.isInt())
>> +    C = llvm::ConstantInt::get(getLLVMContext(), AV.getInt());
>> +  else if (AV.isFloat())
>> +    C = llvm::ConstantFP::get(getLLVMContext(), AV.getFloat());
> 
> Passing the constantint will eliminate this code.
> 
>> +  if (C)
>> +    Dbg->EmitGlobalVariable(C, E->getDecl(), Builder);
> 
> This is calling EmitGlobalVariable *every* time a constant is codegen'd in the code.  In your example, ro+ro+ro+ro will cause the global to get emitted 4 times.  I realize that MDNodes do autouniquing, but doesn't this sound inefficient?

I am not anticipating any measurable compile time penalty in real world scenarios here. However, we could add a SmallPtrSet to keep track of emitted constants at CGDebugInfo level. This means overhead for EmitGlobalVariable() call will remain. What do you think ?

-
Devang