[PATCH] D155654: [RISCV] Add SchedRead for Merge operands on MASK Pseudos

[Wang Pengcheng via Phabricator via llvm-commits]

wangpc added a comment.

In D155654#4515365 <https://reviews.llvm.org/D155654#4515365>, @michaelmaitland wrote:

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



================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:113
+  !cast<SchedReadWrite>("WriteVLDE_" # lmul), ReadVLDX, ReadVMask,
+  !cast<SchedRead>("ReadVMergeOp_" # lmul)
+]>;
----------------
craig.topper wrote:
> Don't the SchedReads need to be in the same order as the `ins` operands for the instruction?
Yes, they must be.
FYI, there are some useful discussions in https://reviews.llvm.org/D123578#inline-1189297.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155654/new/

https://reviews.llvm.org/D155654