[PATCH] Debug info: Support fragmented variables.

[Chandler Carruth]

  Two high-level comments:

  1) This needs a set of direct SROA tests. I don't think these need to use the C-compiled-to-debug stuff as you can use stub metadata nodes (i've tried this in a couple of other tests and it works fine) and just check that the intrinsics are being adjusted the way you want and re-pointed in the way you want. You *might* need one layer of the debug part, but I generally feel like you can make this testable in a more "normal" IR test kind of way.

  2) Clang format. =] (ok, this is a minor comment)

  Detailed questions below. The most important one is trying to understand how you handle un-split cases and iteratively split cases.


================
Comment at: lib/Support/GraphWriter.cpp:90
@@ -89,3 +89,3 @@
     }
-    sys::fs::remove(Filename);
+    //sys::fs::remove(Filename);
     errs() << " done. \n";
----------------
Uh, huh?

================
Comment at: lib/Transforms/Scalar/SROA.cpp:690
@@ -678,1 +689,3 @@
+      DbgDeclare(0)
+{
   SliceBuilder PB(DL, AI, *this);
----------------
Bad reformatting here. Generally, I would encourage you to use clang-format.

================
Comment at: lib/Transforms/Scalar/SROA.cpp:3173
@@ -3143,3 +3172,3 @@
   AllocaSliceRewriter Rewriter(*DL, S, *this, AI, *NewAI, BeginOffset,
-                               EndOffset, IsVectorPromotable,
+                               EndOffset, DIB, IsVectorPromotable,
                                IsIntegerPromotable);
----------------
Why is it important to thread a single DIBuilder through all of these levels? Why not just create the DIBuilder in the AllocaSliceRewriter?

Or if it is expensive to construct, create it as a member?

================
Comment at: lib/Transforms/Scalar/SROA.cpp:2236
@@ +2235,3 @@
+
+    // Split up the debug information if this stores only part of the value.
+    if (DbgDeclareInst *DbgDecl = S.getDbgDeclare()) {
----------------
What happens if the original store was already a split? This will happen a lot with SROA when we iterate on the same alloca.

Also, what about when this *isn't* a split at all? I feel like the first patch should just migrate dbg.declares from old allocas to new allocas when we're rewriting uses but not re-slicing anything.

================
Comment at: lib/Transforms/Scalar/SROA.cpp:2239
@@ +2238,3 @@
+      DIVariable Var(DbgDecl->getVariable());
+      uint64_t Size = EndOffset - BeginOffset;
+      if (Var.getType().getSizeInBits() < Size*8 /* Assuming 8 Bits/Byte */) {
----------------
Note that you would want SliceSize here, as if we split up the store itself, BeginOffset and EndOffset won't reflect that split.

You should add a test case for this as well -- example would be storing the low and high parts of an i64 separately, as well as storing the whole thing.

================
Comment at: lib/Transforms/Scalar/SROA.cpp:426-429
@@ -419,1 +425,6 @@
 
+    // Make a best effort to find a dbg.declare intrinsic describing
+    // the alloca by peeking at the next instruction.
+    if (DbgDeclareInst *DDI=dyn_cast_or_null<DbgDeclareInst>(SI.getNextNode()))
+      S.DbgDeclare = DDI;
+
----------------
This seems like a total hack.

It papers over the fact that this is not relevant for a particular store, but for the entire alloca.

Why can't we find this by locating the use of the alloca in the metadata and then the intrinsic using the metadata? I know it's a non-standard edge in the IR, but I was pretty sure it was still *an* edge in the IR somewhere that would let us find this?

================
Comment at: lib/Transforms/Scalar/SROA.cpp:2241-2244
@@ +2240,6 @@
+      if (Var.getType().getSizeInBits() < Size*8 /* Assuming 8 Bits/Byte */) {
+        DIVariable Piece = DIB.createVariablePiece(Var, BeginOffset, Size);
+        DIB.insertDbgValueIntrinsic(V, 0, Piece, Store)
+          ->setDebugLoc(DbgDecl->getDebugLoc());
+        Pass.DeadInsts.insert(DbgDecl);
+      }
----------------
I'm probably missing something, but it *looks* like this has the real possibility of taking code which has no partial info (no dbg.value calls) and making it suddenly have more info. Is that right? Is that good? Should we be looking for relevant dbg.value calls pertaining to the original alloca and slicing them up rather than just synthesizing our own?

Also, what about memset? memcpy? speculated stores around phis? I assume follow-up patches, but it might be good to document some of that.


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