[PATCH] D70597: [PHIEliminate] skip dbg instruction when LowerPHINode

[Paul Robinson via Phabricator via llvm-commits]

probinson added inline comments.


================
Comment at: llvm/lib/CodeGen/PHIElimination.cpp:210
   // Get an iterator to the last PHI node.
+  // Skip debug instruction to avoid impacting PHI lower.
   MachineBasicBlock::iterator LastPHIIt =
----------------
lower -> lowering


================
Comment at: llvm/lib/CodeGen/PHIElimination.cpp:212
   MachineBasicBlock::iterator LastPHIIt =
-    std::prev(MBB.SkipPHIsAndLabels(MBB.begin()));
+    std::prev(MBB.SkipPHIsLabelsAndDebug(MBB.begin()));
 
----------------
Hmmm I think this does work, however it looks like the interface to LowerPHINode is strange.
- here we get the prev() of first non-PHI/etc
- LowerPHINode does next() to get first non-PHI/etc to compute AfterPHIsIt

Wouldn't it be simpler to have the caller pass AfterPHIsIt directly?
And easier to understand.

Note this suggestion is an API refactoring that could be done separately, your choice.


================
Comment at: llvm/test/CodeGen/X86/phi-node-elimination-dbg-invariant.mir:2
+# RUN: llc -mtriple=x86_64-- -run-pass phi-node-elimination -o - %s | FileCheck %s
+# Debug instruction should not impact PHI node lower when
+# it is at the top of the specified block
----------------
lower -> lowering


================
Comment at: llvm/test/CodeGen/X86/phi-node-elimination-dbg-invariant.mir:11
+body:             |
+  ; Verify PHI lowering without Debug instruction exist
+  ; CHECK-LABEL: name: test1
----------------
`without a Debug instruction.` 


================
Comment at: llvm/test/CodeGen/X86/phi-node-elimination-dbg-invariant.mir:33
+
+  ; Verify PHI lowering with Debug instruction exist
+  ; it is at the top of the specified block
----------------
`with a Debug instruction at the top of the specified block.`


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

https://reviews.llvm.org/D70597