Page MenuHomePhabricator

[MIR][MachineCSE] Implementing proper MachineInstr::getNumExplicitDefs()

Authored by rtereshin on Apr 13 2018, 3:14 PM.



Apparently, MachineInstr class definition as well as pretty much all of
the machine passes assume that the only kind of MachineInstr's operands
that is variadic for variadic opcodes is explicit non-definitions.

In particular, this assumption is made by MachineInstr::defs(), uses(),
and explicit_uses() methods, as well as by MachineCSE pass.

The assumption is incorrect judging from at least TableGen backend
implementation, that recognizes variable_ops in OutOperandList, and the
very existence of G_UNMERGE_VALUES generic opcode, or ARM load multiple
instructions, all of which have variadic defs.

In particular, MachineCSE pass breaks MIR with CSE'able G_UNMERGE_VALUES
instructions in it.

This commit implements MachineInstr::getNumExplicitDefs() similar to
pre-existing MachineInstr::getNumExplicitOperands(), fixes
MachineInstr::defs(), uses(), and explicit_uses(), and fixes MachineCSE

As the issue addressed seems to affect only machine passes that could be
ran mid-GlobalISel pipeline at the moment, the other passes aren't fixed
by this commit, like MachineLICM: that could be done on per-pass basis
when (if ever) they get adopted for GlobalISel.

Diff Detail


Event Timeline

rtereshin created this revision.Apr 13 2018, 3:14 PM
This comment was removed by rtereshin.
This comment was removed by rtereshin.
arsenm accepted this revision.Jun 7 2018, 1:14 PM
arsenm added a subscriber: arsenm.


365 ↗(On Diff #142472)

Somewhat related, I think these need to be renamed to explicit_defs(). I've fixed similar bugs before from not checking the implicit def physical registers

555–556 ↗(On Diff #142472)

I would probably add a separate getNumDefs for this

528–529 ↗(On Diff #142472)

Maybe needs a comment explaining implicit operands always follow explicit

This revision is now accepted and ready to land.Jun 7 2018, 1:14 PM
rtereshin added inline comments.Jun 12 2018, 9:27 AM
365 ↗(On Diff #142472)

I agree, will make it a separate commit.

uses is also quite confusing, because it's actually [explicit uses, implicit defs, implicit uses] range and therefore iterating over it for uses needs to be guarded with additional checks, while all the other ranges are precise and don't need such checks.

Well, "precise" with an assumption that "use" means everything but a register definition, that is register uses, immediates, etc. Which is inconsistent with MachineOperand::isUse predicate that asserts isReg() along the way.

Looks like in practice the majority of MachineInstr clients just gave up and opted out to iterating over all of the operands and doing all the checks they deem necessary. Sadly.

555–556 ↗(On Diff #142472)

Will do.

528–529 ↗(On Diff #142472)

Will add.

This revision was automatically updated to reflect the committed changes.