[PATCH] D70676: [DebugInfo] Don't repeatedly created undef DBG_VALUEs during machine-sinking

[Jeremy Morse via Phabricator via llvm-commits]

jmorse marked 4 inline comments as done.
jmorse added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:140
+    struct SunkDebugDef {
+      SinkingVarSet InstsToSink; /// Set of DBG_VALUEs to recreate.
+      MachineInstr *MI;          /// Location to place DBG_VALUEs.
----------------
aprantl wrote:
> Either
> ```
>  struct SunkDebugDef {
>       /// Set of DBG_VALUEs to recreate.
>       SinkingVarSet InstsToSink; 
>     /// Location to place DBG_VALUEs.
>       MachineInstr *MI;          
>     };
> ```
> or
> ```
>  struct SunkDebugDef {
>       SinkingVarSet InstsToSink; ///< Set of DBG_VALUEs to recreate.
>       MachineInstr *MI;          ///< Location to place DBG_VALUEs.
>     };
> ```
Picked the former,


================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:210
+    /// in this function.
+    void reinsertSunkDebugDefs(MachineFunction &MF);
+
----------------
aprantl wrote:
> I find the nomenclature very confusing.
> 
> In line 137 the comment makes it sound as if
> ```
> %foo = some_insn
> DBG_VALUE %foo, "x"
> DBG_VALUE %foo, "y"
> ```
> %foo is a DebugDef and the two DB_VALUEs are insts, but here it sounds more like the DBG_VALUEs are DebugDefs ... can this be unified?
Yeah, that seems to have been bogus. I've switched the data structures to be "SunkVRegDef"s, which should be closer to the truth, it is the instruction that defines the vreg which is sinking. I've rewritten the comments around the data structures, and renamed this function: it's inserting DBG_VALUEs, and it's the VReg definitions that have sunk.

This means the data structures doesn't have "Debug" in their names, but it should be clear from the comments that they're purely for tracking debugging information.


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

https://reviews.llvm.org/D70676