Page MenuHomePhabricator

[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

Repository
rL LLVM

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 ↗(On Diff #179246)

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 ↗(On Diff #179246)

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

test/CodeGen/X86/twoaddr-dbg-value.mir
12 ↗(On Diff #179246)

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 ↗(On Diff #179246)

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
12 ↗(On Diff #179246)

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 ↗(On Diff #179246)

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
12 ↗(On Diff #179246)

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 ↗(On Diff #179818)

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 ↗(On Diff #179818)

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.