[llvm-commits] Add scope info in DebugLoc

[Bill Wendling]

On Thu, May 7, 2009 at 5:40 PM, Argiris Kirtzidis <akyrtzi at gmail.com> wrote:
> The attached patch introduces 'DebugScope' which gets embedded in DebugLoc.
> This allows scope info to be readily available (along with line info) for
> each machine instruction, and will pull the DwarfWriter out of the
> instruction selectors.
>
> Please review and let me know what you think!
>
Apologies for this being late. My review is inlined:

Index: include/llvm/CodeGen/DebugLoc.h
===================================================================
--- include/llvm/CodeGen/DebugLoc.h	(revision 71198)
+++ include/llvm/CodeGen/DebugLoc.h	(working copy)
@@ -21,17 +21,50 @@
 namespace llvm {
   class GlobalVariable;

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

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

Index: include/llvm/CodeGen/FastISel.h
===================================================================
--- include/llvm/CodeGen/FastISel.h	(revision 71198)
+++ include/llvm/CodeGen/FastISel.h	(working copy)
@@ -57,6 +57,7 @@
   MachineFrameInfo &MFI;
   MachineConstantPool &MCP;
   DebugLoc DL;
+  DebugScope DbgScope;
   const TargetMachine &TM;
   const TargetData &TD;
   const TargetInstrInfo &TII;
@@ -86,6 +87,12 @@
   /// getCurDebugLoc() - Return current debug location information.
   DebugLoc getCurDebugLoc() const { return DL; }

+  void setCurDebugScope(DebugScope ds) { DbgScope = ds; }
+  DebugScope getCurDebugScope() const { return DbgScope; }
+
+  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.

   /// SelectInstruction - Do "fast" instruction selection for the given
   /// LLVM IR instruction, and append generated machine instructions to
   /// the current block. Return true if selection was successful.

Index: lib/CodeGen/MachineFunction.cpp
===================================================================
--- lib/CodeGen/MachineFunction.cpp	(revision 71198)
+++ lib/CodeGen/MachineFunction.cpp	(working copy)
@@ -420,6 +421,19 @@
   return DebugLocInfo.DebugLocations[Idx];
 }

+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? Actually, why
subtract one here at all? In the assert, you can verify that Idx <=
DbgScopeInfos.size().

Index: lib/CodeGen/SelectionDAG/SelectionDAGBuild.cpp
===================================================================
--- lib/CodeGen/SelectionDAG/SelectionDAGBuild.cpp	(revision 71198)
+++ lib/CodeGen/SelectionDAG/SelectionDAGBuild.cpp	(working copy)
@@ -312,6 +312,9 @@
             !StaticAllocaMap.count(cast<AllocaInst>(I)))
           InitializeRegForValue(I);

+  // FIXME: Proper debug scope.
+  DebugScope DbgScope;

What's stopping you from using a proper debug scope here?

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

Thanks, Argiris!

-bw