This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Begin removing hasDummyMask.
ClosedPublic

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

Details

Summary

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
idea.

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.

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
3210

this comment's mis-formatted

llvm/lib/Target/RISCV/RISCVMCInstLower.cpp
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
llvm/lib/Target/RISCV/RISCVMCInstLower.cpp
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)
  OutMI.addOperand(MCOperand::createReg(RISCV::NoRegister))

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.