[llvm-commits] [llvm] r63008 - in /llvm/trunk: include/llvm/CodeGen/DebugLoc.h include/llvm/CodeGen/MachineFunction.h lib/CodeGen/MachineFunction.cpp

[Chris Lattner]

On Jan 25, 2009, at 11:41 PM, Evan Cheng wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=63008&view=rev
> Log:
> Add data structure to define and track debug location during codegen.

Thanks for working on this Evan, this is an important area to get right.

>
> +
> +  /// DebugLocTuple - Debug location tuple of filename id, line and  
> column.
> +  ///
> +  struct DebugLocTuple {
> +    unsigned FileId, Line, Col;
> +
> +    DebugLocTuple(unsigned fi, unsigned l, unsigned c)
> +      : FileId(fi), Line(l), Col(c) {};
> +  };

I think this looks good, and should be kept separate from the dwarf  
data-structures.

> +  /// DebugLoc - Debug location id. This is carried by SDNode and
> +  /// MachineInstr to index into a vector of unique debug location  
> tuples.

Please mention that it defaults to 'invalid' but can be set to  
'noinfo' explicitly.

> +  class DebugLoc {
> +    unsigned Idx;
> +
> +  public:
> +    DebugLoc() : Idx(~0U) {}
> +
> +    static DebugLoc getNoDebugLoc()   { DebugLoc L; L.Idx = 0;    
> return L; }
> +    static DebugLoc get(unsigned idx) { DebugLoc L; L.Idx = idx;  
> return L; }
> +
> +    bool isInvalid() { return Idx == ~0U; }
> +    bool isUnknown() { return Idx == 0; }

These should be const.  If you want to use 'isUnknown', please change  
getNoDebugLoc() to getUnknownLoc(), and please comment the accessor to  
say that this is true when there is no debug info for a statement.

> +  };
> +
> +  struct DebugLocTupleDenseMapInfo {

Please partially specialize DenseMapInfo instead of defining a new  
struct.

> +  typedef DenseMap<DebugLocTuple, unsigned,  
> DebugLocTupleDenseMapInfo>
> +    DebugIdMapType;

Do you need this typedef?  DenseMap<DebugLocTuple, unsigned> isn't too  
crazy.

> +  /// DebugLocTracker - This class tracks debug location information.
> +  ///
> +  struct DebugLocTracker {
> +    // NumFilenames - Size of the DebugFilenames vector.
> +    //
> +    unsigned NumFilenames;
> +
> +    // DebugFilenames - A vector of unique file names.
> +    //
> +    std::vector<std::string> DebugFilenames;

Instead of the vector having a copy of the strings please keep a  
pointer to the string entries in DebugFilenamesMap.  I actually just  
wrote this very similar code for clang:

...
   llvm::StringMap<unsigned, llvm::BumpPtrAllocator> FilenameIDs;
   std::vector<llvm::StringMapEntry<unsigned>*> FilenamesByID;
..
unsigned LineTableInfo::getLineTableFilenameID(const char *Ptr,  
unsigned Len) {
   // Look up the filename in the string table, returning the pre- 
existing value
   // if it exists.
   llvm::StringMapEntry<unsigned> &Entry =
     FilenameIDs.GetOrCreateValue(Ptr, Ptr+Len, ~0U);
   if (Entry.getValue() != ~0U)
     return Entry.getValue();

   // Otherwise, assign this the next available ID.
   Entry.setValue(FilenamesByID.size());
   FilenamesByID.push_back(&Entry);
   return FilenamesByID.size()-1;
}

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/include/llvm/CodeGen/MachineFunction.h (original)
> +++ llvm/trunk/include/llvm/CodeGen/MachineFunction.h Mon Jan 26  
> 01:41:49 2009
> @@ -302,6 +306,15 @@
> +  // 
> = 
> = 
> =-------------------------------------------------------------------- 
> ===//
> +  // Debug location.
> +  //
> +
> +  /// lookUpDebugLocId - Look up the DebugLocTuple index with the  
> given
> +  /// filename, line, and column. It may add a new filename and / or
> +  /// a new DebugLocTuple.
> +  unsigned lookUpDebugLocId(const char *Filename, unsigned Line,  
> unsigned Col);

How about getOrCreateDebugLocID(...)

that would be more consistent with other APIs.

>
> };
>
> // 
> = 
> = 
> =-------------------------------------------------------------------- 
> ===//
>
> Modified: llvm/trunk/lib/CodeGen/MachineFunction.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineFunction.cpp?rev=63008&r1=63007&r2=63008&view=diff
>
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/lib/CodeGen/MachineFunction.cpp (original)
> +++ llvm/trunk/lib/CodeGen/MachineFunction.cpp Mon Jan 26 01:41:49  
> 2009
> @@ -378,6 +378,33 @@
>   return *mc;
> }
>
> +/// lookUpDebugLocId - Look up the DebugLocTuple index with the given
> +/// filename, line, and column. It may add a new filename and / or
> +/// a new DebugLocTuple.
> +unsigned MachineFunction::lookUpDebugLocId(const char *Filename,  
> unsigned Line,
> +                                           unsigned Col) {
> +  unsigned FileId;
> +  StringMap<unsigned>::iterator I =
> +    DebugLocInfo.DebugFilenamesMap.find(Filename);

Please use code similar to what I pasted above, it will be a bit more  
efficient.

-Chris