Page MenuHomePhabricator

MachineScheduler: Restrict macroop fusion to data-dependent instructions.
ClosedPublic

Authored by MatzeB on Jun 25 2015, 1:22 PM.

Details

Summary

Before creating a schedule edge to encourage MacroOpFusion check that:

  • The predecessor actually writes a register that the branch reads.
  • The predecessor has no successors in the ScheduleDAG so we can schedule it in front of the branch.

This avoids skewing the scheduling heuristic in cases where macroop
fusion cannot happen anyway.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB updated this revision to Diff 28495.Jun 25 2015, 1:22 PM
MatzeB retitled this revision from to MachineScheduler: Avoid pointless macroop fusion edges..
MatzeB updated this object.
MatzeB edited the test plan for this revision. (Show Details)
MatzeB added a reviewer: atrick.
MatzeB set the repository for this revision to rL LLVM.
MatzeB added a subscriber: Unknown Object (MLST).

Looks generally OK to me, with one comment. Arguably the HasDataDep check should be done inside of shouldScheduleAdjacent as it is potentially possibly that there's some target that does instruction fusion on instructions without a data dependency, but neither of the two targets that currently define shouldScheduleAdjacent do, and it would be a rather strange thing to do as well, so I think it's OK.

lib/CodeGen/MachineScheduler.cpp
1362–1382 ↗(On Diff #28495)

According to MCInstrDesc.h def operands are always registers, so inverting this to loop over the defs of Other would simplify things. You can also replace the inner loop with MachineInstr::readsRegister, which looks like it does the subregister checking that you want when passed a TargetRegisterInfo.

Looks generally OK to me, with one comment. Arguably the HasDataDep check should be done inside of shouldScheduleAdjacent as it is potentially possibly that there's some target that does instruction fusion on instructions without a data dependency, but neither of the two targets that currently define shouldScheduleAdjacent do, and it would be a rather strange thing to do as well, so I think it's OK.

I haven't heard of any architecture that fuses unrelated operations. If such a thing comes up then we can still add an option to the Mutator or the respective target can simply create a mutator of its own.

lib/CodeGen/MachineScheduler.cpp
1362–1382 ↗(On Diff #28495)

Unfortunately MachineInstr::defs() only includes the explicit register defs, there may be additional implicit defs later in the operand list.

john.brawn added inline comments.Jul 17 2015, 10:08 AM
lib/CodeGen/MachineScheduler.cpp
1362–1382 ↗(On Diff #28495)

Ah, I see. I think you could still use MachineInstr::definesRegister instead of the inner loop though?

MatzeB updated this revision to Diff 30056.Jul 17 2015, 5:26 PM
MatzeB retitled this revision from MachineScheduler: Avoid pointless macroop fusion edges. to MachineScheduler: Restrict macroop fusion to data-dependent instructions..

Good point John, this new version uses MachineInstr::modifiesRegister() instead of a manual loop.

john.brawn accepted this revision.Jul 20 2015, 7:16 AM
john.brawn added a reviewer: john.brawn.

LGTM

This revision is now accepted and ready to land.Jul 20 2015, 7:16 AM
This revision was automatically updated to reflect the committed changes.