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

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

I don't know that my opinion holds much weight, but I agree that reducing the number of nearly-equivalent-but-subtly-different intrinsics/pseudo-instructions for debug-information would make it easier for a new dev to understand their semantics, and make it much easier to extend. I would argue that using different representations based on complexity doesn't make the result any more comprehensible, it just adds more things to comprehend.

I'm still catching up on what has already been discussed/decided here and in the RFC, as well as the history of debug info in LLVM, because AMD is working on extensions to DWARF to support heterogeneous debugging in a standard way (i.e. to support AMD GPU, Intel GPU, NVidia GPU, etc.; the intro at https://llvm.org/docs/AMDGPUDwarfExtensionsForHeterogeneousDebugging.html#introduction does a good job of providing an overview of what we are thinking). Some of the concepts from there seem like they can simplify parts of the debug info handling while also supporting more expressiveness.

One big example of where this is needed is to represent address-spaces when working with memory locations. With the "implicit dereference" model in the current IR intrinsics, and the offset field in the MIR instruction, this becomes fairly complex. By making things explicit in the expression most operations on it are simplified.

Would it be appropriate to propose having a discussion somewhere on the topic with anyone interested, preferably on Discord or somewhere we can talk? I think we are leaning towards a very similar end-state, and I worry about just working in a silo until I can post an RFC to hash this out in text.



================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:495
+  template <typename Operand, typename Instruction>
+  static ArrayRef<Operand *> getDebugOperandsForReg(Instruction *MI,
+                                                    Register Reg) {
----------------
It doesn't seem like you can implement this interface (at least without using static storage somewhere); it is `static`, the return type is non-owning, and none of the inputs provide the storage.

Can this just be replaced with a new "filter" iterator type? The caller can always collect the results into a container if they prefer.


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:504
+    }
+    return Ops;
+  }
----------------
I.e. here ArrayRef doesn't copy or move anything out of `Ops`, and then `Ops` goes out of scope and the ArrayRef is dangling.


================
Comment at: llvm/lib/CodeGen/MachineInstr.cpp:2097
+  if (MCID.Opcode == TargetOpcode::DBG_VALUE)
+    return BuildMI(MF, DL, MCID, IsIndirect, MOs[0], Variable, Expr);
+
----------------
Can this assert on the size of `MOs`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82363