[PATCH] Debug info: Support fragmented variables.

Chandler Carruth chandlerc at gmail.com
Mon Mar 24 12:08:25 PDT 2014


  Mostly commenting on the SROA things.


================
Comment at: lib/Transforms/Scalar/SROA.cpp:2116-2117
@@ +2115,4 @@
+        unsigned VarSize = Var.getSizeInBits(TypeIdentifierMap) / 8;
+        assert(BeginOffset+SliceSize <= VarSize &&
+               "slice is larger than or outside of variable");
+        DIVariable Piece = (SliceSize == VarSize)
----------------
This being an assert worries me. Does the verifier ensure this?

================
Comment at: lib/Transforms/Scalar/SROA.cpp:2124
@@ +2123,3 @@
+        DbgDeclares.insert(std::make_pair(&NewAI,cast<DbgDeclareInst>(NewDDI)));
+        Pass.DeadInsts.insert(DbgDecl);
+      }
----------------
This doesn't make sense to me. We may see the same DbgDecl many times? Wouldn't this become dead only when OldAI became dead?

================
Comment at: lib/Transforms/Scalar/SROA.cpp:970-972
@@ -968,1 +969,5 @@
 
+  /// Debug intrinsics do not show up like regular uses in the
+  /// IR. This side-table holds the missing use edges.
+  DenseMap<Value *, DbgDeclareInst *> DbgDeclares;
+
----------------
If these are actually indexed by AllocaInst*s, why not use that as the key type?

================
Comment at: lib/Transforms/Scalar/SROA.cpp:975-976
@@ +974,4 @@
+  /// Holds a map from DITypeRef to the actual MDNode.
+  DITypeIdentifierMapCache TypeIdentifierMapCache;
+  Optional<DITypeIdentifierMap> TypeIdentifierMap;
+
----------------
This is a pretty terrible interface... Why not just lazily create this and store it in the Module so that the Module provides a direct method to produce a TypeIdentifierMap?

Also, why Optional rather than a pointer? I assume TypeIdentifierMap is some kind of smart pointer?

================
Comment at: lib/Transforms/Scalar/SROA.cpp:2107-2108
@@ -2089,1 +2106,4 @@
 
+    if (&NewAI != &OldAI) {
+      // Migrate debug information to the new alloca.
+      if (DbgDeclareInst *DbgDecl = DbgDeclares.lookup(&OldAI)) {
----------------
I don't think this belongs in the slice rewriter. The rewriter is about rewriting uses in the slice vector.

There is a perfect place for this in the rewritePartition method right after we create the new AllocaInst.


http://llvm-reviews.chandlerc.com/D2680



More information about the llvm-commits mailing list