[PATCH] D45878: [DEBUG INFO] Fixing cases where debug info (-g) causes changes in the program.

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

Could you please split this up into one patch per functional change and upload each of them with a MIR testcase? If you don't have them already you can probably just take an existing IR testcase and run the debugify pass on it and then llc it with -stop-after-regalloc  it to synthesize a testcase.



================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:395
 
+/// collectDebgValues - Scan instructions following MI and collect any
+/// matching DBG_VALUEs.
----------------
Please don't repeat the function name in the comment.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:400
+  DbgValues.clear();
+  if (!MI.getOperand(0).isReg())
+    return;
----------------
Please comment why this early exit is necessary.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:403
+
+  MachineBasicBlock::iterator DI = MI; ++DI;
+  for (MachineBasicBlock::iterator DE = MI.getParent()->end();
----------------
Please clang-format.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:407
+    if (!DI->isDebugValue())
+      return;
+    if (DI->getOperand(0).isReg() &&
----------------
For a function that is called collectDebugValues it looks suspicious that this is a return instead of a continue. Could you please add a comment explaining why this is correct?


================
Comment at: lib/CodeGen/MachineSink.cpp:1166
+
+    // collect matching debug values.
+    SmallVector<MachineInstr *, 2> DbgValuesToSink;
----------------
`// Collect ...`



================
Comment at: lib/Target/SystemZ/SystemZElimCompare.cpp:190
 
+/// collectDebgValues - Scan instructions following MI and collect any
+/// matching DBG_VALUEs.
----------------
see above


================
Comment at: lib/Target/SystemZ/SystemZElimCompare.cpp:192
+/// matching DBG_VALUEs.
+static void collectDebugValues(MachineInstr &MI,
+                               SmallVectorImpl<MachineInstr *> &DbgValues) {
----------------
This looks like the same function as above. Factor it out into a general utility?


================
Comment at: lib/Target/SystemZ/SystemZElimCompare.cpp:256
+  for (++MBBI; MBBI != MBBE; ++MBBI) {
+    if (MBBI->isDebugValue())
+      continue;
----------------
Please comment why they are being skipped and and perhaps use isMetaInstruction() instead?


================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:291
+  for (auto &I : *Pred1)
+    if (!isa<DbgValueInst>(I))
+      Size1++;
----------------
isMetaInstruction?


https://reviews.llvm.org/D45878