[PATCH] D67398: [DebugInfo] LiveDebugValues: Move DBG_VALUE creation into VarLoc class

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:286
+      const auto &IID = MI.getDesc();
+      auto *Var = MI.getDebugVariable();
+      auto *DIExpr = MI.getDebugExpression();
----------------
nit: here we have enough horizontal space to actually write DILocalVariable and DIExpression, which may make this slightly easier to read.


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:287
+      auto *Var = MI.getDebugVariable();
+      auto *DIExpr = MI.getDebugExpression();
+
----------------
this seems to be used only in one switch case, perhaps sink it?


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:320
+
     // Is the Loc field a constant or constant object?
     bool isConstant() const { return Kind == ImmediateKind; }
----------------
///


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:360
+        dbgs() << "(null))\n";
+    }
 #endif
----------------
It would be nice if this also printed the kind that we constructed it as (CopyLoc, ...) maybe using an asserts-only kind field? Maybe this also isn't necessary :-)


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:650
       Out << " MI: ";
-      VL.dump();
+      VL.dump(TRI);
     }
----------------
we should probably pass Out into dump and give a dbgs() default argument?


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:780
     VarLoc::SpillLoc SpillLocation = extractSpillBaseRegAndOffset(MI);
-    auto *SpillExpr = DIExpression::prepend(DebugInstr->getDebugExpression(),
-                                            DIExpression::ApplyOffset,
-                                            SpillLocation.SpillOffset);
-    NewDebugInstr = BuildMI(
-        *MF, DebugInstr->getDebugLoc(), DebugInstr->getDesc(), true,
-        SpillLocation.SpillBase, DebugInstr->getDebugVariable(), SpillExpr);
-    VarLoc VL(*NewDebugInstr, SpillLocation.SpillBase,
-              SpillLocation.SpillOffset, LS, *DebugInstr);
-    ProcessVarLoc(VL, NewDebugInstr);
-    LLVM_DEBUG(dbgs() << "Creating DBG_VALUE inst for spill: ";
-               NewDebugInstr->print(dbgs(), /*IsStandalone*/false,
-                                    /*SkipOpers*/false, /*SkipDebugLoc*/false,
-                                    /*AddNewLine*/true, TII));
+    VarLoc VL(*DebugInstr, SpillLocation.SpillBase,
+              SpillLocation.SpillOffset, LS);
----------------
should we have a self-documenting static constructor function for this case as well?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67398/new/

https://reviews.llvm.org/D67398