Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[RISCV] Begin removing hasDummyMask.

Authored by craig.topper on Jun 2 2023, 7:22 PM.



This was used to know if we need to insert a dummy operand during
MCInstLowering. We can use the operand info from MCInstrDesc to
figure this out without needing a separate flag.

I'll remove the tablegen bits if there is consensus this is a good

Diff Detail

Event Timeline

craig.topper created this revision.Jun 2 2023, 7:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2023, 7:22 PM
craig.topper requested review of this revision.Jun 2 2023, 7:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2023, 7:22 PM
Herald added subscribers: eopXD, MaskRay. · View Herald Transcript

Makes sense to me as an approach.


this comment's mis-formatted

226 ↗(On Diff #528066)

This could maybe be a bit safer? If OutMI had more operands than the MCID accounts for, this access would do bad things.

Maybe rejigging that following assert so that it first asserts that OutMI has one fewer operand than OutMCID? That doesn't negate the need for the later assert, if we're conditionally adding the extra operand. Maybe moving that later assert to just before the return statement as a more global check that we've done the right thing throughout the course of the function?

reames added inline comments.Jun 12 2023, 8:17 AM
226 ↗(On Diff #528637)

You can reuse OutNumOperands here. Or remove it, and fold the call into it's single use. It's mostly the inconsistency which is odd.

227 ↗(On Diff #528637)

I'm guessing this if should actually be an assert? If the RegClass didn't match, we'd fail the operand check just below.

Or alternatively, maybe reorganize the whole clause as:

if (OutMI.getNumOperands() + 1 == OutMCID.getNumOperands() &&
   OutMCID.operands()[OutNumOperands].RegClass == RISCV::VMV0RegClassID)

The single assert at the end of the function then covers all the other cases.

Rebase and address comments

This revision was not accepted when it landed; it landed in state Needs Review.Jun 12 2023, 12:02 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.