[PATCH] IR: Move the complex address expression field out of DIVariable and into an extra argument of the dbg.declare/dbg.value intrinsics.

[David Blaikie]

================
Comment at: include/llvm/CodeGen/MachineModuleInfo.h:394
@@ -392,3 +393,3 @@
   /// information of a variable.
-  void setVariableDbgInfo(MDNode *N, unsigned Slot, DebugLoc Loc) {
-    VariableDbgInfo Info = { N, Slot, Loc };
+  void setVariableDbgInfo(MDNode *Expr, MDNode *Var,
+                          unsigned Slot, DebugLoc Loc) {
----------------
I'd probably put the Var at the start - sort of the broadest thing first. But it's not a strong preference & certainly no great argument from consistency. (& I won't repeat this feedback for all the various similar APIs that have the same ordering choice)

================
Comment at: include/llvm/IR/DebugInfo.h:722
@@ +721,3 @@
+/// DIExpression - A complex location expression.
+class DIExpression : public DIDescriptor {
+  friend class DIDescriptor;
----------------
Does this benefit/need to be a DIDescriptor? I suppose in dumping code it might be handy (?) but generally we're never going to have a generic DIDescriptor and have to ask if it's a DIExpression. There's only one place we'll find them while navigating the debug info metadata - and that one place there won't be anything else we expect to find.

================
Comment at: include/llvm/IR/Metadata.h:34
@@ -33,3 +33,3 @@
 enum LLVMConstants : uint32_t {
-  DEBUG_METADATA_VERSION = 1  // Current debug info version number.
+  DEBUG_METADATA_VERSION = 2  // Current debug info version number.
 };
----------------
Did we ever figure out the answer to rolling the version number? I assume this is one of the reasons you have so much test fallout - is there any way we can commit this change separately? (I realize that'll leave a revision or two that don't make sense) just from a review-ability perspective? (not that I've looked over the tests - but it'd possibly be nicer to be able to at least eyeball a few that actually have schema changes versus those that just need the revision number updated)

================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:619
@@ -618,3 +618,3 @@
 
-  DIVariable V(MI->getOperand(2).getMetadata());
+  DIVariable V = MI->getDebugVariable();
   if (V.getContext().isSubprogram()) {
----------------
This looks like cleanup (could be a separate, precursor patch), no?

================
Comment at: lib/IR/DIBuilder.cpp:1087
@@ +1086,3 @@
+/// createExpression - Create an empty DIExpression.
+DIExpression DIBuilder::createExpression() {
+  return DIExpression(MDNode::get(VMContext, ArrayRef<Value *>()));
----------------
What's this for? (I haven't seen any callers)

If needed, I'd just implement it by having the above function default the argument to None:

createExpression(Arrayref<Value *> Addr = None)

http://reviews.llvm.org/D4919