[llvm-commits] Add scope info in DebugLoc

[Argiris Kirtzidis]

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.

> +  /// 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 ?

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

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

>  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.

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

> @@ -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!


-Argiris
-------------- next part --------------
A non-text attachment was scrubbed...
Name: debugscope.patch
Type: text/x-diff
Size: 21354 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20090518/8f5c062a/attachment.patch>