[PATCH] Debug info: Support fragmented variables.

Chandler Carruth chandlerc at gmail.com
Mon Dec 22 11:49:17 PST 2014


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


================
Comment at: lib/Transforms/Scalar/SROA.cpp:3265-3267
@@ +3264,5 @@
+      DIExpression Piece;
+      if (DL->getTypeAllocSize(SliceTy) !=
+          DL->getTypeAllocSize(AI.getAllocatedType()))
+        Piece = DIB->createPieceExpression(BeginOffset, SliceSize);
+      Instruction *NewDDI = DIB->insertDeclare(NewAI, Var, Piece, &AI);
----------------
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.

================
Comment at: lib/Transforms/Scalar/SROA.cpp:3728
@@ -3703,1 +3727,3 @@
+
+  DIB.reset(new DIBuilder(*F.getParent(), /*AllowUnresolved*/ false));
 
----------------
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. =]

================
Comment at: test/DebugInfo/X86/sroasplit-1.ll:3
@@ +2,3 @@
+;
+; rdar://problem/15928306
+;
----------------
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.

http://reviews.llvm.org/D2680

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list