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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added inline comments.


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:262
+      VarLoc VL(MI, LS);
+      VL.Kind = EntryValueKind;
+      VL.Expr = EntryExpr;
----------------
djtodoro wrote:
> Is this redundant, since we already set the Kind within the `VarLoc`'s constructor?
This is deliberately building a VarLoc from the register location in `MI`, then modifying it to be an entry value.

This could be done differently, but I thought a function that "turns this existing DBG_VALUE into an entry-value VarLoc" would be simpler than building a whole VarLoc from scratch. I'll update the comment to indicate that's what I intended.


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:287
+      auto *Var = MI.getDebugVariable();
+      auto *DIExpr = MI.getDebugExpression();
+
----------------
aprantl wrote:
> this seems to be used only in one switch case, perhaps sink it?
It's passed to BuildMI in each switch case (I've moved EntryValueKind to not fall-through with the latest revision).


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:360
+        dbgs() << "(null))\n";
+    }
 #endif
----------------
aprantl wrote:
> 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 :-)
Ah well, determining where a VarLoc originates from is the topic of a future patch, best leave it til then when it'll be easier to implement.


================
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);
----------------
aprantl wrote:
> should we have a self-documenting static constructor function for this case as well?
Sounds good


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

https://reviews.llvm.org/D67398