[PATCH] D83890: [DebugInfo] Process DBG_VALUE_LIST in LiveDebugValues

Mon Jul 20 01:13:55 PDT 2020

djtodoro added inline comments.

================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:263
+// Simple Set for storing all the VarLoc Indices at a Location bucket.
+using VarLocsInRange = SmallSet<LocIndex::u32_index_t, 32>;
+// Vector of all `LocIndex`s for a given VarLoc; the same Location should not
----------------
StephenTozer wrote:
> djtodoro wrote:
> > In stands for Indices here? I'd change that in order not to mix up terms with the In/Out...
> In this case, "In" just means "In", as in the VarLocs that are in a given range. I think there could be a better name for this as well as some of the other classes here; it might even be better to make them into full classes rather than just named sets/vectors. I'm not sure whether doing so would make it easier to understand what's going on, or just make it harder to compare to the previous behaviour.
OK. I thinks In/Out are well known already here, so I think we don't have to worry about that. We might want to update the comment above to remove any ambiguity. 

================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:308

-    enum VarLocKind {
+    enum class MachineLocKind {
       InvalidKind = 0,
----------------
StephenTozer wrote:
> djtodoro wrote:
> > I don't understand why we need new Kind for the EntryValue here.
> After completing the overall work I found the separation isn't strictly necessary, as entry values currently only ever consist of a single register. The logical distinction is that a complete VarLoc is or is not an EntryValue; a machine location doesn't have EntryValue-ness. Still: at the moment we lose no information by combining them, although if at any point we expand what is permissible for an entry value they should be separated.
That makes sense then. There are targets where the entry value would be in multiple regs, but I am not sure we can face it with the current support. I am OK with the separation.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83890