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

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

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



================
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.
----------------
(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?


================
Comment at: llvm/lib/CodeGen/MachineInstr.cpp:2124
+                                SmallVectorImpl<MachineInstr *> &DbgValues) {
+  MachineInstr &MI = *this;
+  if (!MI.getOperand(0).isReg() || MI.getOperand(0).getReg() == 0)
----------------
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.


================
Comment at: llvm/lib/CodeGen/MachineInstr.cpp:2128
+
+  auto &MRI = this->getParent()->getParent()->getRegInfo();
+
----------------
I think you definitely can skip "this->", but also "getParent()->getParent()->" since there is a  getRegInfo() helper in MachineInstr.




================
Comment at: llvm/lib/CodeGen/MachineInstr.cpp:2130
+
+  for (auto &UseMI : MRI.use_instructions(MI.getOperand(0).getReg())) {
+    if (UseMI.isDebugValue() && getParent() == UseMI.getParent())
----------------
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.



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

https://reviews.llvm.org/D58453