This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Skip over dbg-instr in twoaddr pass
ClosedPublic

Authored by markus on Dec 21 2018, 1:08 AM.

Details

Summary

A DBG_VALUE between a two-address instruction and a following COPY would prevent rescheduleMIBelowKill optimization inside TwoAddressInstructionPass.

Diff Detail

Event Timeline

markus created this revision.Dec 21 2018, 1:08 AM
bjope added a subscriber: bjope.Dec 21 2018, 6:28 AM
aprantl added inline comments.
lib/CodeGen/TwoAddressInstructionPass.cpp
932–937

For IR we have an iterator that skips over debug intrinsics automatically. Does the same thing exist for MIR, too?

vsk added a subscriber: vsk.Dec 21 2018, 8:33 PM

Thanks for the patch!

lib/CodeGen/TwoAddressInstructionPass.cpp
932–937

Yes, I believe we have skipDebugInstructionsForward / skipDebugInstructionsBackward. It might be neat to use that here.

test/CodeGen/X86/twoaddr-dbg-value.mir
13

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.

markus added inline comments.Dec 27 2018, 12:04 AM
lib/CodeGen/TwoAddressInstructionPass.cpp
932–937

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?

test/CodeGen/X86/twoaddr-dbg-value.mir
13

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.

vsk added inline comments.Dec 31 2018, 11:31 AM
lib/CodeGen/TwoAddressInstructionPass.cpp
932–937

I personally find this easier to follow. Imho it's nice that the isDebugInstr check doesn't need to be duplicated.

test/CodeGen/X86/twoaddr-dbg-value.mir
13

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.

markus updated this revision to Diff 179818.Jan 1 2019, 11:55 PM
markus marked an inline comment as done.Jan 2 2019, 12:06 AM
markus added inline comments.
lib/CodeGen/TwoAddressInstructionPass.cpp
933

Of course if 'End == MBB->end()' after this point bad things would happen, but on the other hand that would also have been problematic in the original loop (and apparently it was not a problem in practice). Perhaps simply adding an assertion to indicate that this has been considered would be sufficient?

aprantl accepted this revision.Jan 2 2019, 11:05 AM
aprantl added inline comments.
lib/CodeGen/TwoAddressInstructionPass.cpp
933

The last instruction pretty much has to be a terminator instruction and can't really be a debug intrinsic.

This revision is now accepted and ready to land.Jan 2 2019, 11:05 AM
This revision was automatically updated to reflect the committed changes.