Debug info: Support fragmented variables.

[aprantl]

Summary

Reposting in Phabricator as requested.

Currently, LLVM cannot represent locations for aggregate variables that
are fragmented across multiple Values. This situation arises, e.g.,
during SROA, or even as part of the x86_64 calling convention when a struct
is passed by value. This patch adds the functionality to emit
DWARF location expressions using DW_OP(_bit)_piece to express partial
values.

This is implemented by adding a new operation type OpPiece to complex
DIVariables which accepts an offset and a size.

The canonical example would be something like this:

typedef struct { long int a; int b;} S;

int foo(S s) {
     return s.b;
}

which at -O1 is now codegen’d into:

; Function Attrs: nounwind readnone ssp uwtable
define i32 @foo(i64 %s.coerce0, i32 %s.coerce1) #0 {
entry:
 tail call void @llvm.dbg.value(metadata !{i64 %s.coerce0}, i64 0, metadata !20)
 tail call void @llvm.dbg.value(metadata !{i32 %s.coerce1}, i64 0, metadata !21)
 ret i32 %s.coerce1, !dbg !24
}

with this patch we'd emit the following DWARF:

0x00000047:         TAG_formal_parameter [3]  
                  AT_name( "s" )
                  AT_decl_file( "struct.c" )
                  AT_decl_line( 3 )
                  AT_type( {0x00000069} ( S ) )
                  AT_location( 0x00000000
                     0x0000000000000000 - 0x0000000000000008: rdi, piece 0x00000008
                     0x0000000000000000 - 0x0000000000000006: rsi, bit-piece 32 64 
                     0x0000000000000006 - 0x0000000000000008: rax, bit-piece 32 64  )

cheers,
adrian

[dblaikie]

Could you show a little more of your example (you show the function's IR, but not the debug info metadata associated with it (I'm OK if you gloss over most of the metadata details, but highlighting the new metadata constructs your adding would be helpful - rather than wandering through the full test case to get the general idea))

Also, a high level (higher than the code itself) description of the strategy you've taken would be helpful. I'm particularly curious about the use of a "entire" DIVariable and how/why it's built from constituent parts.

[aprantl]

Hi David,

Here’s the MDNodes for the two variable pieces:

!20 = metadata !{i32 786689, metadata !4, metadata !"s", metadata !5, i32 16777219, metadata !9, i32 0, i32 0, i32 3, i32 0, i32 8} ; [ DW_TAG_arg_variable ] [s] [line 3]
!21 = metadata !{i32 786689, metadata !4, metadata !"s", metadata !5, i32 16777219, metadata !9, i32 0, i32 0, i32 3, i32 8, i32 4} ; [ DW_TAG_arg_variable ] [s] [line 3]

All the way through ISel and the MC layer those two MDNodes behave like regular DIVariables with complex addresses. In DwarfDebug, we need to treat the pieces as one entity when calculating the Ranges/History. getEntireVariable() simply chops off the last two elements so it can be used as a shared index into DbgVariables while still indicating that this is a fragmented location. The extra code in the range calculation makes sure that a piece of a variable is not clobbered by a non-overlapping piece.

FYI: In a previous design I experimented with introducing a new type MDnode

!Piece = metadata !{i32 PIECE, metadata !<ref to DIVariable>, metadata !<Offset>, metadata !<Size>}

but this meant adding an extra case to all the ISel/MC code handling DBG_VALUEs, which is why I ended up with the current design.

— adrian

[aprantl]

Ping! Does anyone have a strong opinion on this design?

thanks,

• adrian

[echristo]

I've got a few comments on the code, but there's no update for the documentation in SourceLevelDebugging.html. How about we get that solidified first so we've got a good idea of what format we're looking at for the metadata.

include/llvm/DebugInfo.h

684 ↗

(On Diff #6836)

Cut and paste.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1305 ↗

(On Diff #6836)

Would probably be nice to have a function for getDebugMetadata or something for MachineInstr instead of the subtraction.

1320 ↗

(On Diff #6836)

Typo in function name.

lib/Transforms/Scalar/SROA.cpp

426

Can't you just get the users here?

947

Instead of passing all of these down it should be possible to construct one.

[aprantl]

Added documentation and fixed typos.

[aprantl]

Rebased patch on ToT and addressed review comments. Most importantly this resolves a conflict with r201190, which added DW_OP_piece support for variables occupying only part of a register (essentially the complementary problem).

[aprantl]

Updated to also support variables split up by SelectionDAG’s integer type legalization. Thanks iains for providing additional testcases.

[echristo]

Hi Adrian,

This is a lot of great work, thanks!

I think the first question is how this is going to interact with DIBuilder::createComplexVariable? I think it should just be a concatenation of the two things. It looks like you have it as an instead? In general, I think I like the idea that variable locations just have expressions attached to them. And lacking anything else I think dwarf expressions are just fine here.

I've got more comments on the patch itself, but let's get this out of the way first.

-eric

[chandlerc]

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.

lib/Support/GraphWriter.cpp

90 ↗

(On Diff #7324)

Uh, huh?

lib/Transforms/Scalar/SROA.cpp

426–429

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?

690

Bad reformatting here. Generally, I would encourage you to use clang-format.

2236

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.

2239

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.

2241–2244

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.

3173

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?

[aprantl]

On Feb 26, 2014, at 3:06, Chandler Carruth <[e-mail]> wrote:

Hi Chandler/Eric/David,

Thanks for your feedback, everyone!
I think I addressed every comment now, below are some notes regarding the non-obvious or controversial changes.

Two high-level comments:

1. This needs a set of direct SROA tests.

There are now three tests that explicitly run opt -sroa and check the IR output. The IR output is then also compiled with llc to verify the dwarf output, so they are still located in the tests/DebugInfo directory.

Detailed questions below. The most important one is trying to understand how you handle un-split cases and iteratively split cases.
...
What happens if the original store was already a split? This will happen a lot with SROA when we iterate on the same alloca.

This is not an issue because we can create a variable piece from an existing variable piece just fine.

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.

I moved the code that migrates the debug declares from the Store handler to the AllocaSlice visitor of AllocaRewriter. It also handles the case were the alloca is just rewritten without slicing. Currently it’s using the DIType of the DIVariable to determine whether this should be a VariablePiece or not. This is not strictly necessary, though: it forces us to build a DITypeIdentifierMap in SROA, but it saves us 3 int32 per variable MDNode when the alloca is not split. Alternatively we could also just always create pieces, and perform the check whether a piece covers the entire variable back in AsmPrinter.

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?

The current implementation uses a DenseMap<AllocaInst, DbgDeclare> to find the debug intrinsic for an alloca.

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?

Maybe I’m misunderstanding what you said there, but as far as I understand it this:

Should we be looking for relevant dbg.value calls pertaining to the original alloca and slicing them up rather than just synthesizing our own?

is describing exactly what we are doing right now. Also note that only dbg.declare instrinsics can describe allocas.

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

This is not yet complete and I plan to add other cases over time!

[aprantl]

Here is a rebased version of the patch that applies cleanly after Chandler’s movement of header files and Eric’s creation of AsmPrinterDwarf.

[chandlerc]

(sorry for delays, trying to get to this one...)

[echristo]

(Ditto)

[aprantl]

Rebased on r204198.
Moved the DWARF expression output for DW_op_pieces out of emitDebugLocEntry::emitDebugLocEntry() and into a DWARFPieceEmitter. Should be more readable this way.

[aprantl]

Hi Eric & Chandler,

it’s been a while, and I was just thinking about what we could do to make this move forward a little faster. I understand that the more time is passing the patch grows larger and large which makes it harder to review. We could, for instance split out the SROA changes into a separate patch, and then focus on the IR and DwarfDebug changes initially, with the type legalizer providing the necessary test cases.

cheers,
adrian

[chandlerc]

Mostly commenting on the SROA things.

lib/Transforms/Scalar/SROA.cpp

970–972

If these are actually indexed by AllocaInst*s, why not use that as the key type?

975–976

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?

2107–2108

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.

2116–2117

This being an assert worries me. Does the verifier ensure this?

2124

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?

[echristo]

Let's look into splitting this up and out a bit. I think being able to have complex addresses as part of a location in the debug value instruction should be fine. The multiple optional statements bother me a bit, I guess we'll need to add a null metadata in cases where you want to use this.

Let's split the patch out with just the infrastructure changes needed and a testcase to use them and we'll go from there? In general, I think it's a good idea, just a big patch :)

-eric

[echristo]

Let's look into splitting this up and out a bit. I think being able to have complex addresses as part of a location in the debug value instruction should be fine. The multiple optional statements bother me a bit, I guess we'll need to add a null metadata in cases where you want to use this. What about turning it into a non-optional argument? Just keep a NULL for a complex address? How badly does that affect the existing tests?

Let's split the patch out with just the infrastructure changes needed and a testcase to use them and we'll go from there? In general, I think it's a good idea, just a big patch :)

i'd probably split out the "optimize" as well. Yeah, we'll emit non-optimal for a couple of patches, but as an add-on that seems like a more obvious patch.

I echo Chandler's thoughts about the type uniquing map. What's going on there?

[echristo]

Let's raise this back to everyone's queue again. Had some questions on the type uniquing map, but I think most of the prereqs were already reviewed and so this can be rebased?

[aprantl]

Rebased on today's trunk. Otherwise mostly unchanged so far, I have a couple of questions regarding Chandler's comments that I'd like to clarify before the next iteration.
Thanks for all the feedback so far!

[dblaikie]

I'm curious why there are both SROA and SelDAG changes in this together - I assume the SelDAG (and any other LLVM CodeGen) changes could be separated/tested with raw IR inputs and the SROA changes should be implemented/tested as standalone IR->IR behavior, right?

So is there more separation to do in this patch, or are these changes somehow inextricably linked?

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1285 ↗

(On Diff #12121)

Remove commented out code (& unrelated whitespace changes) - then this file shouldn't be included in the diff at all.

Ideally this whole change shouldn't have any changes to core debug info APIs/IR APIs - those changes have already been committed. This change is now just about teaching certain optimizations to preserve/create the IR necessary to describe the variable location fragments, right?

[aprantl]

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

lib/Transforms/Scalar/SROA.cpp

970–972

They could also be function arguments, not just allocas.

975–976

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.

2107–2108

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.

2116–2117

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.

2124

The goal is to replace the single DbgDecl that describes the old alloca with many smaller dbg instrinsics that describe the individual slices.

[aprantl]

Splitting out TypeLegalizer changes into a separate review.

[aprantl]

The type legalizer changes can be found at http://reviews.llvm.org/D4831.

[dblaikie]

And both of these patches are predicated on http://reviews.llvm.org/D3373 right?

[aprantl]

Removed extra whitespace from patch.

[dblaikie]

Bumping this hopefully into Chandler's queue (by setting him as a reviewer, rather than just a subscriber) and some minor feedback from me on the test cases.

test/DebugInfo/X86/sroasplit-2.ll

9

Perhaps these two test cases could be named more precisely for what they do - it took me a bit of staring to see the specific difference between them (the type of the Inner::b member is different) and I still don't have any idea why that's important to test separately (what is it that's different about that case)?

(if it is different and different code is needed for it, usual question applies about "is this different code that can be implemented/tested/committed separately", but perhaps it isn't)

& do both cases need the two levels of indirection (a struct, with an array of other structs)? While a test case that covers a few cases in one go is soemtimes nice, it'd help me at least if there was a comment explaining why the different pieces of indirection were important to the test - what parts of SROA they're exercising.

[aprantl]

Rebased on todays trunk.

This patch teaches SROA to split the debug info for aggregate variables into pieces describing the individual scalar elements of the former aggregate.

Chandler, could you please review my responses to you previous review?

[aprantl]

Fix an initializer for MSVC compatibility.

[chandlerc]

To finish reviewing this, it would help to have a full context patch? If that's a problem, I can patch it locally and take a look...

lib/Transforms/Scalar/SROA.cpp

959

These are of Value *s, but what Values? The loads? The allocas? Something else?

Essentially, I think this comment needs to indicate what the synthetic "use" edge looks like for the purpose of SROA.

2117–2118

The order here seems a bit odd. I would group the references separately from the builders....

Also, nit-picky issue, clang-format please. =]

3724

Bad indentation...

[aprantl]

Rebased on trunk and addressed Chandler's feedback.

[chandlerc]

Thanks for the full context diff, it did help. =D

lib/Transforms/Scalar/SROA.cpp

2119

You don't need this -- the SROA pass object is available directly as "Pass".

2179–2190

Actually, why do this here at all? This doesn't seem to really be related to rewriting the uses in the traditional sense. Notably, it seems excessive if OldAI == NewAI (which happens in some cases).

I think the better place to put this logic is inside the rewritePartition method of the SROA pass itself right after we create the new alloca.

3725–3726

This should be a dyn_cast to AllocaInst and a re-use.

Also, getAddress can return null which seems like it would crash here. Unless the verifier rejects such an debug declare inst (and if it does, it would be nice to have an "unsafe" version of the method that returns null for the verifier and have this method assert for us that the result is non-null) this should handle the null case gracefully.

Somewhat separately (and apologies if this just is my ignorance of debug info) what cases would you expect where the address isn't directly an alloca inst?

[aprantl]

lib/Transforms/Scalar/SROA.cpp

2119

Thanks!

2179–2190

Done.

3725–3726

See the first testcase:

define i32 @foo(%struct.Outer* byval align 8 %outer) #0 {
  call void @llvm.dbg.declare(metadata %struct.Outer* %outer, metadata !25, metadata !2), !dbg !26

it could be an argument.

[aprantl]

Addressed Chandler's feedback / moved the debug intrinsic generation right after we create the new alloca.

[aprantl]

Changed isa/cast to dyn_cast.

[aprantl]

lib/Transforms/Scalar/SROA.cpp

3725–3726

No the verifier does not reject it, although an UndefValue would be preferred over of a nullptr. I'll change it to a dyn_cast_or_null.

[aprantl]

s/dyn_cast/dyn_cast_or_null/

[chandlerc]

The code is looking quite good at this point. I'm mostly looking for some cleanups / clarifications of things I don't understand.

lib/Transforms/Scalar/SROA.cpp

3265–3267

This predicate is somewhat subtle. You're creating a piece expression any time the alloc size isn't the same, but you're using the slice size rather than the alloc size to create the piece.... Can you add a comment here? If I came to this code later, I wouldn't understand why the different sizes used were significant.

3728

Is there a particular cost to constructiog DIBuilder objects? I ask because, were this an IRBuilder, I would have said to just construct it directly in each place we need it, because there is no real cast associated with it. If the same is true for DIBuilder, then I'd suggest the same change. If DIBuilder works differently, I'd like to know how to better use it. =]

test/DebugInfo/X86/sroasplit-1.ll

4

Honestly, I really dislike seeing rdar links even in tests. They add no information for any reader not at Apple, and so you end up needing to explain all of the context of the test anyways. PR links are at least a reasonable thing for your average contributor to go read.

[aprantl]

lib/Transforms/Scalar/SROA.cpp

3265–3267

I changed condition and comment to this, which should be more clear:

// Create a piece expression describing the slice, if the new slize is
// smaller than the old alloca or the old alloca already was described
// with a piece. It would be even better to just compare against the size
// of the type described in the debug info, but then we would need to
// build an expensive DIRefMap.
if (SliceSize < DL->getTypeAllocSize(AI.getAllocatedType()) ||
    DbgDeclares[AI].getExpression().isVariablePiece())
  Piece = DIB->createPieceExpression(BeginOffset, SliceSize);

3728

Compared to an IRBuilder it looks as if DIBuilder has to initialize quite a few members (that will never be used). But I'm unsure whether there actually is a cost to initializing DenseMaps and SmallVectors? Otherwise I'd be happy to instantiate the DIBuilder on the fly. Alternatively we could factor out a less capable DIBuilder base class that does not have all these members.

Module &M;
LLVMContext &VMContext;

MDNode *TempEnumTypes;
MDNode *TempRetainTypes;
MDNode *TempSubprograms;
MDNode *TempGVs;
MDNode *TempImportedModules;

Function *DeclareFn;     // llvm.dbg.declare
Function *ValueFn;       // llvm.dbg.value

SmallVector<Metadata *, 4> AllEnumTypes;
/// Track the RetainTypes, since they can be updated later on.
SmallVector<TrackingMDNodeRef, 4> AllRetainTypes;
SmallVector<Metadata *, 4> AllSubprograms;
SmallVector<Metadata *, 4> AllGVs;
SmallVector<TrackingMDNodeRef, 4> AllImportedModules;

/// \brief Track nodes that may be unresolved.
SmallVector<TrackingMDNodeRef, 4> UnresolvedNodes;
bool AllowUnresolvedNodes;

/// Each subprogram's preserved local variables.
DenseMap<MDNode *, std::vector<TrackingMDNodeRef>> PreservedVariables;

test/DebugInfo/X86/sroasplit-1.ll

4

I can understand that. Removed.

[aprantl]

Address previous round of feedback.

[chandlerc]

LGTM with a minor change below, thanks for all the work explaining things. =]

lib/Transforms/Scalar/SROA.cpp

3728

Interesting.

I suggest you just build them on the fly here, and if these members ever end up being a problem, we can go in and avoid their cost much as you describe. No need to do that until we see a problem, but it seems nicer than having a heap-allocated builder.

[aprantl]

Thanks for the review so far!
This

• Initializes DIBuilder on the fly.
• Improves the DebugInfo/X86/array2.ll testcase based on the new capabilities
• Fixes a compile error in the above-cited expression.

[aprantl]

Closing. Thanks everyone!!

lib/Transforms/Scalar/SROA.cpp

   /// currently in the promotable queue.
   SetVector<SelectInst *, SmallVector<SelectInst *, 2> > SpeculatableSelects;
 
+  /// Debug intrinsics do not show up like regular uses in the
+  /// IR. This side-table holds the missing use edges.
+  DenseMap<Value *, DbgDeclareInst *> DbgDeclares;
+
 public:
   SROA(bool RequiresDomTree = true)
       : FunctionPass(ID), RequiresDomTree(RequiresDomTree),

   // the insertion point is set to point to the user.
   IRBuilderTy IRB;
 
+  DenseMap<Value *, DbgDeclareInst *>& DbgDeclares;
+  DIBuilder DIB;
+
 public:
   AllocaSliceRewriter(const DataLayout &DL, AllocaSlices &AS, SROA &Pass,
                       AllocaInst &OldAI, AllocaInst &NewAI,

                       uint64_t NewAllocaEndOffset, bool IsIntegerPromotable,
                       VectorType *PromotableVecTy,
                       SmallPtrSetImpl<PHINode *> &PHIUsers,
-                      SmallPtrSetImpl<SelectInst *> &SelectUsers)
+                      SmallPtrSetImpl<SelectInst *> &SelectUsers,
+                      DenseMap<Value *, DbgDeclareInst *>& DbgDeclares)
       : DL(DL), AS(AS), Pass(Pass), OldAI(OldAI), NewAI(NewAI),
         NewAllocaBeginOffset(NewAllocaBeginOffset),
         NewAllocaEndOffset(NewAllocaEndOffset),

         ElementSize(VecTy ? DL.getTypeSizeInBits(ElementTy) / 8 : 0),
         BeginOffset(), EndOffset(), IsSplittable(), IsSplit(), OldUse(),
         OldPtr(), PHIUsers(PHIUsers), SelectUsers(SelectUsers),
-        IRB(NewAI.getContext(), ConstantFolder()) {
+        IRB(NewAI.getContext(), ConstantFolder()),
+        DbgDeclares(DbgDeclares),
+        DIB(*NewAI.getParent()->getParent()->getParent()) {
     if (VecTy) {
       assert((DL.getTypeSizeInBits(ElementTy) % 8) == 0 &&
              "Only multiple-of-8 sized vector elements are viable");

     IRB.SetCurrentDebugLocation(OldUserI->getDebugLoc());
     IRB.SetNamePrefix(Twine(NewAI.getName()) + "." + Twine(BeginOffset) + ".");
 
+    // Migrate debug information from the old alloca to the new alloca
+    // and the individial slices.
+    if (DbgDeclareInst *DbgDecl = DbgDeclares.lookup(&OldAI)) {
+      DIVariable Var(DbgDecl->getVariable());
+      // Create a variable piece to describe the slice. This works fine
+      // even if Var already was a piece.
+      DIExpression Piece = DIB.createPieceExpression(BeginOffset, SliceSize);
+      Instruction *NewDDI = DIB.insertDeclare(&NewAI, Var, Piece, OldPtr);
+      NewDDI->setDebugLoc(DbgDecl->getDebugLoc());
+      DbgDeclares.insert(std::make_pair(&NewAI, cast<DbgDeclareInst>(NewDDI)));
+      Pass.DeadInsts.insert(DbgDecl);
+    }
+
     CanSROA &= visit(cast<Instruction>(OldUse->getUser()));
     if (VecTy || IntTy)
       assert(CanSROA);

 
   AllocaSliceRewriter Rewriter(*DL, AS, *this, AI, *NewAI, BeginOffset,
                                EndOffset, IsIntegerPromotable, VecTy, PHIUsers,
-                               SelectUsers);
+                               SelectUsers, DbgDeclares);
   bool Promotable = true;
   for (auto & SplitUse : SplitUses) {
     DEBUG(dbgs() << "  rewriting split ");

 
   BasicBlock &EntryBB = F.getEntryBlock();
   for (BasicBlock::iterator I = EntryBB.begin(), E = std::prev(EntryBB.end());
-       I != E; ++I)
+       I != E; ++I) {
     if (AllocaInst *AI = dyn_cast<AllocaInst>(I))
       Worklist.insert(AI);
+    else if (DbgDeclareInst *DDI = dyn_cast<DbgDeclareInst>(I))
+    DbgDeclares.insert(std::make_pair(DDI->getAddress(), DDI));
+  }
 
   bool Changed = false;
   // A set of deleted alloca instruction pointers which should be removed from

test/DebugInfo/X86/sroasplit-1.ll

+; RUN: opt %s -sroa -verify -S -o - | FileCheck %s
+;
+; rdar://problem/15928306
+;
+; Test that we can partial emit debug info for aggregates repeatedly
+; split up by SROA.
+;
+;    // Compile with -O1
+;    typedef struct {
+;      int a;
+;      long int b;
+;    } Inner;
+;
+;    typedef struct {
+;      Inner inner[2];
+;    } Outer;
+;
+;    int foo(Outer outer) {
+;      Inner i1 = outer.inner[1];
+;      return i1.a;
+;    }
+;
+
+; Verify that SROA creates a variable piece when splitting i1.
+; CHECK: %[[I1:.*]] = alloca [12 x i8], align 4
+; CHECK: call void @llvm.dbg.declare(metadata !{[12 x i8]* %[[I1]]}, metadata ![[VAR:[0-9]+]], metadata ![[PIECE:[0-9]+]])
+; Read Var and Piece:
+; CHECK: ![[VAR]] = {{.*}} ; [ DW_TAG_auto_variable ] [i1] [line 11]
+; CHECK: ![[PIECE]] = {{.*}} ; [ DW_TAG_expression ] [DW_OP_piece offset=0, size=12]
+
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.9.0"
+
+%struct.Outer = type { [2 x %struct.Inner] }
+%struct.Inner = type { i32, i64 }
+
+; Function Attrs: nounwind ssp uwtable
+define i32 @foo(%struct.Outer* byval align 8 %outer) #0 {
+entry:
+  %i1 = alloca %struct.Inner, align 8
+  call void @llvm.dbg.declare(metadata !{%struct.Outer* %outer}, metadata !25, metadata !2), !dbg !26
+  call void @llvm.dbg.declare(metadata !{%struct.Inner* %i1}, metadata !27, metadata !2), !dbg !28
+  %inner = getelementptr inbounds %struct.Outer* %outer, i32 0, i32 0, !dbg !28
+  %arrayidx = getelementptr inbounds [2 x %struct.Inner]* %inner, i32 0, i64 1, !dbg !28
+  %0 = bitcast %struct.Inner* %i1 to i8*, !dbg !28
+  %1 = bitcast %struct.Inner* %arrayidx to i8*, !dbg !28
+  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %0, i8* %1, i64 16, i32 8, i1 false), !dbg !28
+  %a = getelementptr inbounds %struct.Inner* %i1, i32 0, i32 0, !dbg !29
+  %2 = load i32* %a, align 4, !dbg !29
+  ret i32 %2, !dbg !29
+}
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+; Function Attrs: nounwind
+declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture, i8* nocapture readonly, i64, i32, i1) #2
+
+attributes #0 = { nounwind ssp uwtable }
+attributes #1 = { nounwind readnone }
+attributes #2 = { nounwind }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!22, !23}
+!llvm.ident = !{!24}
+
+!0 = metadata !{metadata !"0x11\0012\00clang version 3.5.0 \000\00\000\00\001", metadata !1, metadata !2, metadata !2, metadata !3, metadata !2, metadata !2} ; [ DW_TAG_compile_unit ] [ DW_TAG_compile_unit ] [sroasplit-1.c] [DW_LANG_C99]
+!1 = metadata !{metadata !"sroasplit-1.c", metadata !""}
+!2 = metadata !{}
+!3 = metadata !{metadata !4}
+!4 = metadata !{metadata !"0x2e\00foo\00foo\00\0010\000\001\000\006\00256\000\0010", metadata !1, metadata !5, metadata !6, null, i32 (%struct.Outer*)* @foo, null, null, metadata !2} ; [ DW_TAG_subprogram ] [ DW_TAG_subprogram ] [line 10] [def] [foo]
+!5 = metadata !{metadata !"0x29", metadata !1} ; [ DW_TAG_file_type ] [ DW_TAG_file_type ] [sroasplit-1.c]
+!6 = metadata !{metadata !"0x15\00\000\000\000\000\000\000", i32 0, null, null, metadata !7, null, null, null} ; [ DW_TAG_subroutine_type ] [ DW_TAG_subroutine_type ] [line 0, size 0, align 0, offset 0] [from ]
+!7 = metadata !{metadata !8, metadata !9}
+!8 = metadata !{metadata !"0x24\00int\000\0032\0032\000\000\005", null, null} ; [ DW_TAG_base_type ] [ DW_TAG_base_type ] [int] [line 0, size 32, align 32, offset 0, enc DW_ATE_signed]
+!9 = metadata !{metadata !"0x16\00Outer\008\000\000\000\000", metadata !1, null, metadata !10} ; [ DW_TAG_typedef ] [ DW_TAG_typedef ] [Outer] [line 8, size 0, align 0, offset 0] [from ]
+!10 = metadata !{metadata !"0x13\00\006\00256\0064\000\000\000", metadata !1, null, null, metadata !11, null, null, null} ; [ DW_TAG_structure_type ] [line 6, size 256, align 64, offset 0] [def] [from ]
+!11 = metadata !{metadata !12}
+!12 = metadata !{metadata !"0xd\00inner\007\00256\0064\000\000", metadata !1, metadata !10, metadata !13} ; [ DW_TAG_member ] [inner] [line 7, size 256, align 64, offset 0] [from ]
+!13 = metadata !{metadata !"0x1\00\000\00256\0064\000\000", null, null, metadata !14, metadata !20, i32 0, null, null, null} ; [ DW_TAG_array_type ] [line 0, size 256, align 64, offset 0] [from Inner]
+!14 = metadata !{metadata !"0x16\00Inner\004\000\000\000\000", metadata !1, null, metadata !15} ; [ DW_TAG_typedef ] [ DW_TAG_typedef ] [Inner] [line 4, size 0, align 0, offset 0] [from ]
+!15 = metadata !{metadata !"0x13\00\001\00128\0064\000\000\000", metadata !1, null, null, metadata !16, null, null, null} ; [ DW_TAG_structure_type ] [line 1, size 128, align 64, offset 0] [def] [from ]
+!16 = metadata !{metadata !17, metadata !18}
+!17 = metadata !{metadata !"0xd\00a\002\0032\0032\000\000", metadata !1, metadata !15, metadata !8} ; [ DW_TAG_member ] [a] [line 2, size 32, align 32, offset 0] [from int]
+!18 = metadata !{metadata !"0xd\00b\003\0064\0064\0064\000", metadata !1, metadata !15, metadata !19} ; [ DW_TAG_member ] [b] [line 3, size 64, align 64, offset 64] [from long int]
+!19 = metadata !{metadata !"0x24\00long int\000\0064\0064\000\000\005", null, null} ; [ DW_TAG_base_type ] [ DW_TAG_base_type ] [long int] [line 0, size 64, align 64, offset 0, enc DW_ATE_signed]
+!20 = metadata !{metadata !21}
+!21 = metadata !{metadata !"0x21\000\002"}        ; [ DW_TAG_subrange_type ] [0, 1]
+!22 = metadata !{i32 2, metadata !"Dwarf Version", i32 2}
+!23 = metadata !{i32 1, metadata !"Debug Info Version", i32 2}
+!24 = metadata !{metadata !"clang version 3.5.0 "}
+!25 = metadata !{metadata !"0x101\00outer\0016777226\000", metadata !4, metadata !5, metadata !9} ; [ DW_TAG_arg_variable ] [ DW_TAG_arg_variable ] [outer] [line 10]
+!26 = metadata !{i32 10, i32 0, metadata !4, null}
+!27 = metadata !{metadata !"0x100\00i1\0011\000", metadata !4, metadata !5, metadata !14} ; [ DW_TAG_auto_variable ] [ DW_TAG_auto_variable ] [i1] [line 11]
+!28 = metadata !{i32 11, i32 0, metadata !4, null}
+!29 = metadata !{i32 12, i32 0, metadata !4, null}

test/DebugInfo/X86/sroasplit-2.ll

+; RUN: opt %s -sroa -verify -S -o - | FileCheck %s
+;
+; rdar://problem/15928306
+;
+; Test that we can partial emit debug info for aggregates repeatedly
+; split up by SROA.
+;
+;    // Compile with -O1
+;    typedef struct {
+;      int a;
+;      int b;
+;    } Inner;
+;
+;    typedef struct {
+;      Inner inner[2];
+;    } Outer;
+;
+;    int foo(Outer outer) {
+;      Inner i1 = outer.inner[1];
+;      return i1.a;
+;    }
+;
+
+; Verify that SROA creates a variable piece when splitting i1.
+; CHECK:  call void @llvm.dbg.value(metadata !{i64 %outer.coerce0}, i64 0, metadata ![[O:[0-9]+]], metadata ![[PIECE1:[0-9]+]]),
+; CHECK:  call void @llvm.dbg.value(metadata !{i64 %outer.coerce1}, i64 0, metadata ![[O]], metadata ![[PIECE2:[0-9]+]]),
+; CHECK:  call void @llvm.dbg.value({{.*}}, i64 0, metadata ![[I1:[0-9]+]], metadata ![[PIECE3:[0-9]+]]),
+; CHECK-DAG: ![[O]] = {{.*}} [ DW_TAG_arg_variable ] [outer] [line 10]
+; CHECK-DAG: ![[PIECE1]] = {{.*}} [ DW_TAG_expression ] [DW_OP_piece offset=0, size=8]
+; CHECK-DAG: ![[PIECE2]] = {{.*}} [ DW_TAG_expression ] [DW_OP_piece offset=12, size=4]
+; CHECK-DAG: ![[I1]] = {{.*}} [ DW_TAG_auto_variable ] [i1] [line 11]
+; CHECK-DAG: ![[PIECE3]] = {{.*}} [ DW_TAG_expression ] [DW_OP_piece offset=0, size=4]
+
+; ModuleID = 'sroasplit-2.c'
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.9.0"
+
+%struct.Outer = type { [2 x %struct.Inner] }
+%struct.Inner = type { i32, i32 }
+
+; Function Attrs: nounwind ssp uwtable
+define i32 @foo(i64 %outer.coerce0, i64 %outer.coerce1) #0 {
+  %outer = alloca %struct.Outer, align 8
+  %i1 = alloca %struct.Inner, align 4
+  %1 = bitcast %struct.Outer* %outer to { i64, i64 }*
+  %2 = getelementptr { i64, i64 }* %1, i32 0, i32 0
+  store i64 %outer.coerce0, i64* %2
+  %3 = getelementptr { i64, i64 }* %1, i32 0, i32 1
+  store i64 %outer.coerce1, i64* %3
+  call void @llvm.dbg.declare(metadata !{%struct.Outer* %outer}, metadata !24, metadata !2), !dbg !25
+  call void @llvm.dbg.declare(metadata !{%struct.Inner* %i1}, metadata !26, metadata !2), !dbg !27
+  %4 = getelementptr inbounds %struct.Outer* %outer, i32 0, i32 0, !dbg !27
+  %5 = getelementptr inbounds [2 x %struct.Inner]* %4, i32 0, i64 1, !dbg !27
+  %6 = bitcast %struct.Inner* %i1 to i8*, !dbg !27
+  %7 = bitcast %struct.Inner* %5 to i8*, !dbg !27
+  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %6, i8* %7, i64 8, i32 4, i1 false), !dbg !27
+  %8 = getelementptr inbounds %struct.Inner* %i1, i32 0, i32 0, !dbg !28
+  %9 = load i32* %8, align 4, !dbg !28
+  ret i32 %9, !dbg !28
+}
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+; Function Attrs: nounwind
+declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture, i8* nocapture readonly, i64, i32, i1) #2
+
+attributes #0 = { nounwind ssp uwtable "no-frame-pointer-elim"="true" }
+attributes #1 = { nounwind readnone }
+attributes #2 = { nounwind }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!21, !22}
+!llvm.ident = !{!23}
+
+!0 = metadata !{metadata !"0x11\0012\00clang version 3.5.0 \000\00\000\00\001", metadata !1, metadata !2, metadata !2, metadata !3, metadata !2, metadata !2} ; [ DW_TAG_compile_unit ] [ DW_TAG_compile_unit ] [sroasplit-2.c] [DW_LANG_C99]
+!1 = metadata !{metadata !"sroasplit-2.c", metadata !""}
+!2 = metadata !{}
+!3 = metadata !{metadata !4}
+!4 = metadata !{metadata !"0x2e\00foo\00foo\00\0010\000\001\000\006\00256\000\0010", metadata !1, metadata !5, metadata !6, null, i32 (i64, i64)* @foo, null, null, metadata !2} ; [ DW_TAG_subprogram ] [ DW_TAG_subprogram ] [line 10] [def] [foo]
+!5 = metadata !{metadata !"0x29", metadata !1} ; [ DW_TAG_file_type ] [ DW_TAG_file_type ] [sroasplit-2.c]
+!6 = metadata !{metadata !"0x15\00\000\000\000\000\000\000", i32 0, null, null, metadata !7, null, null, null} ; [ DW_TAG_subroutine_type ] [ DW_TAG_subroutine_type ] [line 0, size 0, align 0, offset 0] [from ]
+!7 = metadata !{metadata !8, metadata !9}
+!8 = metadata !{metadata !"0x24\00int\000\0032\0032\000\000\005", null, null} ; [ DW_TAG_base_type ] [ DW_TAG_base_type ] [int] [line 0, size 32, align 32, offset 0, enc DW_ATE_signed]
+!9 = metadata !{metadata !"0x16\00Outer\008\000\000\000\000", metadata !1, null, metadata !10} ; [ DW_TAG_typedef ] [ DW_TAG_typedef ] [Outer] [line 8, size 0, align 0, offset 0] [from ]
+!10 = metadata !{metadata !"0x13\00\006\00128\0032\000\000\000", metadata !1, null, null, metadata !11, null, null, null} ; [ DW_TAG_structure_type ] [line 6, size 128, align 32, offset 0] [def] [from ]
+!11 = metadata !{metadata !12}
+!12 = metadata !{metadata !"0xd\00inner\007\00128\0032\000\000", metadata !1, metadata !10, metadata !13} ; [ DW_TAG_member ] [inner] [line 7, size 128, align 32, offset 0] [from ]
+!13 = metadata !{metadata !"0x1\00\000\00128\0032\000\000", null, null, metadata !14, metadata !19, i32 0, null, null, null} ; [ DW_TAG_array_type ] [line 0, size 128, align 32, offset 0] [from Inner]
+!14 = metadata !{metadata !"0x16\00Inner\004\000\000\000\000", metadata !1, null, metadata !15} ; [ DW_TAG_typedef ] [ DW_TAG_typedef ] [Inner] [line 4, size 0, align 0, offset 0] [from ]
+!15 = metadata !{metadata !"0x13\00\001\0064\0032\000\000\000", metadata !1, null, null, metadata !16, null, null, null} ; [ DW_TAG_structure_type ] [line 1, size 64, align 32, offset 0] [def] [from ]
+!16 = metadata !{metadata !17, metadata !18}
+!17 = metadata !{metadata !"0xd\00a\002\0032\0032\000\000", metadata !1, metadata !15, metadata !8} ; [ DW_TAG_member ] [a] [line 2, size 32, align 32, offset 0] [from int]
+!18 = metadata !{metadata !"0xd\00b\003\0032\0032\0032\000", metadata !1, metadata !15, metadata !8} ; [ DW_TAG_member ] [b] [line 3, size 32, align 32, offset 32] [from int]
+!19 = metadata !{metadata !20}
+!20 = metadata !{metadata !"0x21\000\002"}        ; [ DW_TAG_subrange_type ] [0, 1]
+!21 = metadata !{i32 2, metadata !"Dwarf Version", i32 2}
+!22 = metadata !{i32 1, metadata !"Debug Info Version", i32 2}
+!23 = metadata !{metadata !"clang version 3.5.0 "}
+!24 = metadata !{metadata !"0x101\00outer\0016777226\000", metadata !4, metadata !5, metadata !9} ; [ DW_TAG_arg_variable ] [ DW_TAG_arg_variable ] [outer] [line 10]
+!25 = metadata !{i32 10, i32 0, metadata !4, null}
+!26 = metadata !{metadata !"0x100\00i1\0011\000", metadata !4, metadata !5, metadata !14} ; [ DW_TAG_auto_variable ] [ DW_TAG_auto_variable ] [i1] [line 11]
+!27 = metadata !{i32 11, i32 0, metadata !4, null}
+!28 = metadata !{i32 12, i32 0, metadata !4, null}