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

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer marked 3 inline comments as done.
StephenTozer 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
----------------
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.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:308
 
-    enum VarLocKind {
+    enum class MachineLocKind {
       InvalidKind = 0,
----------------
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.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:322
 
-    /// The value location. Stored separately to avoid repeatedly
-    /// extracting it from MI.
-    union {
+    typedef union {
       uint64_t RegNo;
----------------
djtodoro wrote:
> why typedef here?
This union type is useful to have at the scope of VarLoc, since several VarLoc methods use variables with these values. The best alternative I think would be to move it into MachineLoc; regardless, having it as a typedef instead of an anonymous union is convenient in the functions below where it is used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83890