[PATCH] D55987: [CodeGen] Skip over dbg-instr in twoaddr pass

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: lib/CodeGen/TwoAddressInstructionPass.cpp:932
   MachineBasicBlock::iterator End = AfterMI;
-  while (End->isCopy() &&
-         regOverlapsSet(Defs, End->getOperand(1).getReg(), TRI)) {
-    Defs.push_back(End->getOperand(0).getReg());
+  while (End->isDebugInstr() ||
+         (End->isCopy() &&
----------------
markus wrote:
> vsk wrote:
> > aprantl wrote:
> > > For IR we have an iterator that skips over debug intrinsics automatically. Does the same thing exist for MIR, too?
> > Yes, I believe we have skipDebugInstructionsForward / skipDebugInstructionsBackward. It might be neat to use that here.
> If we were to use skipDebugInstructionsForward then it seems that the loop would have to be written as follows to preserve the semantics (and handle multiple COPYs with DBG_VALUEs interspersed in various ways).
> 
> ```
> while (End != MBB->end()) {
>   End = skipDebugInstructionsForward(End, MBB->end())
>   if (End->isCopy() && regOverlapsSet(Defs, End->getOperand(1).getReg(), TRI)))
>     Defs.push_back(End->getOperand(0).getReg());
>   else
>     break;
>   ++End;
> }
> ```
> Would that be better?
I personally find this easier to follow. Imho it's nice that the isDebugInstr check doesn't need to be duplicated.


================
Comment at: test/CodeGen/X86/twoaddr-dbg-value.mir:12
+    %4:gr32 = XOR32ri8 %1, 1, implicit-def dead $eflags
+    DBG_VALUE %4
+    %5:gr32 = COPY %4
----------------
markus wrote:
> vsk wrote:
> > This is mostly an academic question, but: is this a valid DBG_VALUE? I'm a little surprised this parses (I'd expect the instruction to have more operands), but I like the simplicity of it.
> It is probably not valid but does seem to be sufficient for the purpose of this test (where the presence of a DBG_VALUE is the only relevant thing and not the contents of it). I would be happy to make it more valid though if that is just a matter of adding the additional operands.
Ah, I see. I think this is fine the way it is. It'd be a bit messy to add the debug-location metadata, and the benefit of having a fully-formed dbg_value here seems fairly marginal.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55987