[llvm-commits] Add scope info in DebugLoc

[Bill Wendling]

Hi Argiris,

On May 18, 2009, at 1:17 PM, Argiris Kirtzidis wrote:

> Hi Bill,
>
> Bill Wendling wrote:
>> -  /// DebugLocTuple - Debug location tuple of filename id, line  
>> and column.
>> +  /// DebugScope - Debug scope id.
>>
>> Please flesh out this comment. Add more about what a debug scope is
>> and how it will be used.
>>
>> +  class DebugScope {
>> +    unsigned Idx;
>> +
>> +  public:
>> +    DebugScope() : Idx(~0U) {}  // Defaults to null.
>>
>> Well...it's not really Null. :-) Maybe it would be better to name it
>> "invalid" (with appropriate changes to the other methods' names)?
>>
>> +  /// DebugScopeInfo - Contains info about the scope global  
>> variable and the
>> +  /// parent debug scope.
>>
>> Could you explain why this is different from DebugScope.
>>
>
> Ok to all.
>
Thanks! :)

>> +  /// DebugLocTuple - Debug location tuple of compile unit, scope,
>> +  /// line and column.
>>
>> The term "compile unit" is overloaded. There's the idea of a CU in  
>> the
>> debug info, then the actual CompileUnit class, then a DICompileUnit
>> class. Could you name it something else? Just for my sanity? :-) That
>> is, unless CompileUnit is really absolutely correct here...
>>
>
> I modified it to "DICompileUnit global variable", is this more clear ?
>
It is.

>> +  void EnterDebugScope(GlobalVariable *ScopeGV, MachineFunction  
>> &MF);
>> +  void ExitDebugScope(GlobalVariable *ScopeGV, MachineFunction &MF);
>>
>> FastISel has MF available as an ivar. Do you need to pass this in  
>> as a
>> parameter? Also, please comment these methods.
>>
>
> Moved them into a new DebugScopeTracker class.
>
Thank you!

>> +DebugScope MachineFunction::CreateDebugScope(GlobalVariable  
>> *ScopeGV,
>> +                                             DebugScope Parent) {
>> +  DbgScopeInfos.push_back(DebugScopeInfo(ScopeGV, Parent));
>> +  return DebugScope::get(DbgScopeInfos.size() - 1);
>>
>> Should you verify that DbgScopeInfos.size() > 0 here?
>
> Is it really needed ? A new item is pushed into the DbgScopeInfos in  
> the immediate line above it.
>
Ah! You're right. I was blind to it. :-)

>> Actually, why
>> subtract one here at all? In the assert, you can verify that Idx <=
>> DbgScopeInfos.size().
>>
>
> Well, there's not much difference but I'd prefer to have  
> DebugScope::getIndex return the "actual index" into the vector.
> This is similar to the way DebugLoc's index is created.
>
Now that I see that the original code wasn't buggy, this is fine.

>> +  // FIXME: Proper debug scope.
>> +  DebugScope DbgScope;
>>
>> What's stopping you from using a proper debug scope here?
>>
>
> I added comments about this. Let me know what you think about this  
> part.

Looks good!

>> @@ -3817,6 +3821,25 @@
>>   setValue(&I, result);
>> }
>>
>> +void SelectionDAGLowering::EnterDebugScope(GlobalVariable *ScopeGV,
>> +                                           MachineFunction &MF) {
>> +  assert(ScopeGV && "GlobalVariable for scope is null!");
>> +  setCurDebugScope(MF.CreateDebugScope(ScopeGV,  
>> getCurDebugScope()));
>> +}
>> +
>> +void SelectionDAGLowering::ExitDebugScope(GlobalVariable *ScopeGV,
>> +                                          MachineFunction &MF) {
>> +  assert(ScopeGV && "GlobalVariable for scope is null!");
>> +  assert(!getCurDebugScope().isNull() && "Mismatched region.end ?");
>> +  // We may have skipped a region.end because it was in an  
>> unreachable block.
>> +  // Go up the scope chain until we reach the scope that ScopeGV  
>> points to.
>> +  DebugScopeInfo DSI;
>> +  do {
>> +    DSI = MF.getDebugScopeInfo(getCurDebugScope());
>> +    setCurDebugScope(DSI.Parent);
>> +  } while (!DSI.Parent.isNull() && DSI.GV != ScopeGV);
>> +}
>>
>> Because these methods are used in both SelectionDAGBuild.cpp and
>> FastISel.cpp, is there a way to factor them out into another module?
>> Maybe a CodeGen/DebugLoc.cpp module?
>>
>
> Done.
>
> I attached a new patch, thanks for reviewing!
>
The patch looks good. :-) Be careful about 80-column violations (I  
think I saw one potential one?). Okay to commit.

Thank you for working on this!
-bw