This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add SchedRead for Merge operands on MASK Pseudos
ClosedPublic

Authored by michaelmaitland on Jul 18 2023, 3:37 PM.

Details

Summary

For Pseudos that end in _MASK, in the case that the instruction
should be mask undisturbed, the destination register must be read
to determine the undisturbed values. This patch adds a SchedRead
that gets passed to MASK pseudos to represent this extra read that
must occur.

A future patch should do something similiar for when a pseudo is TU,
since those instructions must also read their destination to preserve
undisturbed-ness.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2023, 3:37 PM
michaelmaitland requested review of this revision.Jul 18 2023, 3:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2023, 3:38 PM

This makes sense to me, but please wait for others' comments.
(Just one concern that the schedule model of RVV is becoming more and more complicated)

Just one concern that the schedule model of RVV is becoming more and more complicated

I am happy to have a discussion on what we can do to simplify the model! If you have any ideas on what we can do to simplify, I think that this would be a good topic for a post on discourse, the RISCV bi-weekly meeting, or you can always shoot me over an email :)

I think that the change here models a read that we were definitely missing. The only added complexity of this change is in the definition of ReadVMergeOp_ # mx and ReadVMergeOp_ # mx # "_E" # sew in RISCVScheduleV.td. The rest of the changes are really just moving the Sched into subclasses, which isn't really adding any new complexity.

With respect to reducing complexity, I do think that the behavior of RVV instructions are more complex at its core compared to scalar, and perhaps even to SIMD instructions of other architectures (since a single RVV instruction depends on the global vtype register). If I had to guess, SIMD architectures probably have less pseudos per concrete instruction, but they have more concrete instructions. This makes me skeptical that we're overcomplicating things -- I think RVV has moved the complexity out of its ISA and the compiler has the burden of relocating this complexity into its pseudo system and scheduler in order to make accurate scheduler decisions.

I think that much of the RVV scheduler work that has been done (a) accounts for factors that impact instruction behavior and scheduling which has led us to see performance improvements and (b) we've done so in a way that has been frugal with respect to creating new pseudos and other tablegen records, as to avoid too large of an increase in the size of the toolchain binaries.

craig.topper added inline comments.Jul 19 2023, 9:41 PM
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
103

Don't the SchedReads need to be in the same order as the ins operands for the instruction?

Just one concern that the schedule model of RVV is becoming more and more complicated

I am happy to have a discussion on what we can do to simplify the model! If you have any ideas on what we can do to simplify, I think that this would be a good topic for a post on discourse, the RISCV bi-weekly meeting, or you can always shoot me over an email :)

I think that the change here models a read that we were definitely missing. The only added complexity of this change is in the definition of ReadVMergeOp_ # mx and ReadVMergeOp_ # mx # "_E" # sew in RISCVScheduleV.td. The rest of the changes are really just moving the Sched into subclasses, which isn't really adding any new complexity.

With respect to reducing complexity, I do think that the behavior of RVV instructions are more complex at its core compared to scalar, and perhaps even to SIMD instructions of other architectures (since a single RVV instruction depends on the global vtype register). If I had to guess, SIMD architectures probably have less pseudos per concrete instruction, but they have more concrete instructions. This makes me skeptical that we're overcomplicating things -- I think RVV has moved the complexity out of its ISA and the compiler has the burden of relocating this complexity into its pseudo system and scheduler in order to make accurate scheduler decisions.

I think that much of the RVV scheduler work that has been done (a) accounts for factors that impact instruction behavior and scheduling which has led us to see performance improvements and (b) we've done so in a way that has been frugal with respect to creating new pseudos and other tablegen records, as to avoid too large of an increase in the size of the toolchain binaries.

Yeah, I'm totally agreed with your opinions and thinkings about RVV. The complexity (which comes from register grouping and mask/tail policy mostly I think) of schedule model meets our expectation actually. I have thought about simplifying it, but got no result. :-(
As for your patch, it's really an important/appreciative bugfix (I'm surprised that we missed them, which means that we never get correct schedule model for masked instructions before /facepalm🤦), but some changes are needed as what @craig.topper pointed out.

llvm/lib/Target/RISCV/RISCVInstrInfoV.td
103

Yes, they must be.
FYI, there are some useful discussions in https://reviews.llvm.org/D123578#inline-1189297.

michaelmaitland marked 2 inline comments as done.

Fix SchedReads to be in the same order as the ins operands for the instruction.

Fix SchedReads to be in the same order as the ins operands for the instruction
in RISCVInstrInfoV.

Fix docstring for InsertAtIdx1

Nice! I just have some suggestions oncode style.

llvm/lib/Target/RISCV/RISCVInstrInfoV.td
275

This class VLXSEGSchedMask should be used.

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
2097

We should add some multiclasses to define Scheds (just like existed multiclasses for load/store, e.g., VLESchedMask, ) instead of passing SchedRead/SchedWrites to these multiclasses which are used to define pseudos. They make the code messy, I think.
We can seperate these multiclasses into other patches to make this patch easy to review. I am pleased to help you with this.

wangpc added inline comments.Jul 21 2023, 3:00 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
2097

multiclass->class

Please see D155932, feedback is welcome.

michaelmaitland marked an inline comment as done.

Use VLXSEGSchedMask

llvm/lib/Target/RISCV/RISCVInstrInfoV.td
275

Thanks for catching this

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
2097

I agree with your proposal in D155932. I think this comment can be resolved and this patch will make the required changes after rebasing once D155932 lands?

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
2097

It might be easier to wait to make your changes until this lands, otherwise one of us is going to be stuck rebasing unnecessarily. What do you think?

wangpc added inline comments.Jul 24 2023, 1:47 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
2097

Can you please wait for my patch? I think it will make this patch much simpler. :-)

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
2097

Yes, I will wait and rebase after your changes :)

wangpc accepted this revision.Jul 26 2023, 12:38 AM
wangpc added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
2504

unneeded removal?

This revision is now accepted and ready to land.Jul 26 2023, 12:38 AM
wangpc added inline comments.Jul 26 2023, 3:13 AM
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
114

ReadVMergeOp->readVMergeOp or simply mergeOp?

michaelmaitland marked 2 inline comments as done.
  • Fix unneeded removal
  • Rename ReadVMergeOp to mergeOp
  • Rebase
wangpc added inline comments.Jul 26 2023, 9:42 AM
llvm/lib/Target/RISCV/RISCVSchedSiFive7.td
1157

No need to do casting here, just use ReadVMergeOp_WorstCase.

michaelmaitland marked an inline comment as done.

Remove cast.

wangpc accepted this revision.Jul 26 2023, 9:58 AM

LGTM

This revision was landed with ongoing or failed builds.Jul 26 2023, 10:57 AM
This revision was automatically updated to reflect the committed changes.