[PATCH] D58453: [DebugInfo][CGP] Limit placeDbgValues movement of dbg.value intrinsics

[Jeremy Morse via Phabricator via llvm-commits]

jmorse marked 3 inline comments as done.
jmorse added a comment.

Bjorn wrote:

> Is it possible to do the WebAssembly updates in a separate patch, or is those changes dependent on also changing how CodeGenPrepare works?

I guess they can, and they would get more visibility that way; they don't depend on placeDbgValues happily. I'll try splitting those into a separate patch.



================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:1654
 
-  /// Scan instructions following MI and collect any matching DBG_VALUEs.
+  /// Scan instructions immediately following MI and collect any matching
+  /// DBG_VALUEs.
----------------
bjope wrote:
> (Not something added in this patch, but since we are duplicate this, IMHO kind of weak, interface we might wanna do something about it.)
> 
> The "matching" here is not that clear. I notice that you wrote "this instructions first vreg argument" below.
> But these functions those not even check that the first argument is a vreg. Nor that it is a def. And what it the MI has more than one def?
> 
> One idea is to clearly specify that we only collect DBG_VALUE instructions using the register defined by operand zero (iff that operand is a def). And then the implementation of these functions should bail out if operand zero isn't a register def.
> We could also add a TODO that maybe we should look at all defs in the future.
> 
> Or is it by design that we also collect DGB_VALUEs matching operand zero even if operand zero is a use? Then why aren't we doing this for all uses that this instruction got?
These are all great questions, and I don't have a great explanation for why collectDebugValues is the way it is. IMHO, the first operand being a vreg is an assumption that held in the past, largely hinging on placeDbgValues existing, and all the callers to collectDebugValues guaranteed it themselves.

It's worth noting that I was originally going to completely delete this method, but the RISCV backend has grown a dependency on it, and would be the only user once this patch lands. That code (RISCVISelLower.cpp's emitSelectPseudo) seems to have a good use case, they're moving DBG_VALUEs out of a code sequence that's going to be completely rewritten. It doesn't need any consideration of what the DBG_VALUEs refer to, only that they come immediately after the current / "this" instruction.

How about:
 * Making collectDebugValues not consider operands at all: it becomes a helper to collect DBG_VALUEs that follow an instruction, nothing more.
 * Making collectBlockDebugValues a helper for WebAssembly stuff, as that's the only real consumer.

This means that there's no longer a helper method anywhere that "gets my debug insts". IMO, we should add these when we discover use cases, for example "collectDebugUsersOfDef", which could be defined a lot more precisely.


================
Comment at: llvm/lib/CodeGen/MachineInstr.cpp:2124
+                                SmallVectorImpl<MachineInstr *> &DbgValues) {
+  MachineInstr &MI = *this;
+  if (!MI.getOperand(0).isReg() || MI.getOperand(0).getReg() == 0)
----------------
bjope wrote:
> Not sure what kind of idiom this is. Can't we just skip this MI that referes to this (it isn't used consistently below anyway as you are using "this->" in one place, and getParent() in another place.
> 
> An alternative would be to make the function static, and provide MI as an argument.
I'll polish this and split it into the WebAssembly specific patch.


================
Comment at: llvm/lib/CodeGen/MachineInstr.cpp:2130
+
+  for (auto &UseMI : MRI.use_instructions(MI.getOperand(0).getReg())) {
+    if (UseMI.isDebugValue() && getParent() == UseMI.getParent())
----------------
bjope wrote:
> nit: If we assume that DBG_VALUE never is bundled (we should implement a verifier check for that if it does not exist), this could be use_bundles instead of use_instructions. I'm just saying that collectDebugValues is iterating on a bundle level, but right now this function also scan for DBG_VALUE inside bundles.
> 
Ah, I know approximately nothing about instruction bundling -- DBG_VALUEs never appear inside of a bundle?


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

https://reviews.llvm.org/D58453