[PATCH] Debug info: Support fragmented variables.

[Adrian Prantl]

Added some questions/comments to Chandler's questions. Thanks!

================
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;
+
----------------
Chandler Carruth wrote:
> If these are actually indexed by AllocaInst*s, why not use that as the key type?
They could also be function arguments, not just allocas.

================
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;
+
----------------
Chandler Carruth wrote:
> 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?
The main concern was memory usage. I wanted only create this on demand when it's used and free the memory after the pass is done with the module. That said, I'm not overly attached to this model.

Got rid of the optional.

================
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)) {
----------------
Chandler Carruth wrote:
> 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.
What I'm doing here is replace the one dbg.declare describing the old alloca with one for each of the slices. IF I understand it correctly :-) it appears that rewritePartition would be too early for this because it didn;t yet create the slices yet. 

================
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)
----------------
Chandler Carruth wrote:
> This being an assert worries me. Does the verifier ensure this?
The verifier (DIVariable::Verify()) cannot ensure this, because it would need access to a DITypeIdentifierMap to look up the type. There is, however, the same assertion in DwarfDebug just before the Dwarf expression it would be emitted.

If we *do* make the DITypeIdentifierMap a feature of the module we could have the verifier take care of it.

================
Comment at: lib/Transforms/Scalar/SROA.cpp:2124
@@ +2123,3 @@
+        DbgDeclares.insert(std::make_pair(&NewAI,cast<DbgDeclareInst>(NewDDI)));
+        Pass.DeadInsts.insert(DbgDecl);
+      }
----------------
Chandler Carruth wrote:
> 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?
The goal is to replace the single DbgDecl that describes the old alloca with many smaller dbg instrinsics that describe the individual slices.

http://reviews.llvm.org/D2680