[PATCH] D82363: [DebugInfo] Add new instruction and expression operator for variadic debug values

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:503
+  }
+  ArrayRef<MachineOperand *> getDebugOperandsForReg(Register Reg) {
+    assert(isDebugValue() && "Tried to get debug operands for non-debug_value");
----------------
StephenTozer wrote:
> djtodoro wrote:
> > Can we use templates to avoid duplicated code here for `getDebugOperandsForReg()`?
> We can, as long as we use a static function to hold the common code (if there's a way to do so without a static function then I'd be happy to go with that instead); the solution looks something like this:
> 
> ```
>   template <typename Operand, typename Instruction>
>   static ArrayRef<Operand *> getDebugOperandsForReg(Instruction *MI, Register Reg) {
>     assert(MI->isDebugValue() && "Tried to get debug operands for non-debug_value");
>     SmallVector<Operand *, 2> Ops;
>     for (Operand &Op : MI->debug_operands()) {
>       if (Op.isReg() && Op.getReg() == Reg)
>         Ops.push_back(&Op);
>     }
>     return Ops;
>   }
>   ArrayRef<const MachineOperand *> getDebugOperandsForReg(Register Reg) const {
> 	  return MachineInstr::getDebugOperandsForReg<const MachineOperand, const MachineInstr>(this, Reg);
>   }
>   ArrayRef<MachineOperand *> getDebugOperandsForReg(Register Reg) {
> 	  return MachineInstr::getDebugOperandsForReg<MachineOperand, MachineInstr>(this, Reg);
>   }
> ```
> 
> Does this look good? It removes the duplication, it's just a bit more verbose and leaves an otherwise useless static function hanging around, unless it's moved to a private block (which is also fine but reduces readability by moving it far away from the public functions).
This looks good to me, thanks.


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:486
+  /// register \p Reg.
+  const bool hasDebugOperandForReg(Register Reg) const {
+    return count_if(debug_operands(), [Reg](const MachineOperand &Op) {
----------------
`const` does not have any impact here, since it is a return type, so it should be removed.


================
Comment at: llvm/lib/Target/NVPTX/NVPTXPrologEpilogPass.cpp:79
+          const DIExpression *DIExpr = MI.getDebugExpression();
+          if (MI.isNonVariadicDebugValue()) {
+            DIExpr = DIExpression::prepend(MI.getDebugExpression(),
----------------
Should be `isNonListDebugValue()` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82363