[RISCV] Add SchedRead for Merge operands on MASK Pseudos

[michaelmaitland]

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.

[michaelmaitland]

Rebase on top of D155634

[wangpc]

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)

[michaelmaitland]

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]

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?

[wangpc]

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

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]

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

[michaelmaitland]

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

[michaelmaitland]

Fix docstring for InsertAtIdx1

[wangpc]

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

2172 ↗

(On Diff #542549)

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]

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td

2172 ↗

(On Diff #542549)

multiclass->class

Please see D155932, feedback is welcome.

[michaelmaitland]

Use VLXSEGSchedMask

llvm/lib/Target/RISCV/RISCVInstrInfoV.td

275

Thanks for catching this

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td

2172 ↗

(On Diff #542549)

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?

[michaelmaitland]

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td

2172 ↗

(On Diff #542549)

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]

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td

2172 ↗

(On Diff #542549)

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

[michaelmaitland]

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td

2172 ↗

(On Diff #542549)

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

[michaelmaitland]

Rebase.

[michaelmaitland]

Rebase on top of D155932

[michaelmaitland]

Cleanup diff

[wangpc]

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td

2504 ↗

(On Diff #544081)

unneeded removal?

[wangpc]

llvm/lib/Target/RISCV/RISCVInstrInfoV.td

114

ReadVMergeOp->readVMergeOp or simply mergeOp?

[michaelmaitland]

• Fix unneeded removal
• Rename ReadVMergeOp to mergeOp
• Rebase

[wangpc]

llvm/lib/Target/RISCV/RISCVSchedSiFive7.td

1170

No need to do casting here, just use ReadVMergeOp_WorstCase.

[michaelmaitland]

Remove cast.

[wangpc]

LGTM

llvm/lib/Target/RISCV/RISCVInstrInfoV.td

 def simm5_plus1_nonzero : ImmLeaf<XLenVT,
   [{return Imm != 0 && ((isInt<5>(Imm) && Imm != -16) || Imm == 16);}]>;
 
 //===----------------------------------------------------------------------===//
 // Scheduling definitions.
 //===----------------------------------------------------------------------===//
 
 // Common class of scheduling definitions.
-// `ReadVMask` will be appended if instruction is masked.
+// `ReadVMergeOp` will be prepended to reads if instruction is masked.
+// `ReadVMask` will be appended to reads if instruction is masked.
 // Operands:
 //   `writes`       SchedWrites that are listed for each explicit def operand
 //                  in order.
 //   `reads`        SchedReads that are listed for each explicit use operand.
 //   `forceMasked`  Forced to be masked (e.g. Add-with-Carry Instructions).
 class SchedCommon<list<SchedWrite> writes, list<SchedRead> reads,
-                  bit forceMasked = 0>: Sched<[]> {
+                  string mx = "WorstCase", int sew = 0, bit forceMasked = 0>
+    : Sched<[]> {
   defvar isMasked = !ne(!find(NAME, "_MASK"), -1);
-  defvar needReadVMask = !or(forceMasked, isMasked);
-  let SchedRW = !listconcat(writes, reads,
-                            !if(needReadVMask, [ReadVMask], []));
+  defvar isMaskedOrForceMasked = !or(forceMasked, isMasked);
+  defvar mergeOp = !if(!or(!eq(mx, "WorstCase"), !eq(sew, 0)),
+                            !cast<SchedRead>("ReadVMergeOp_" # mx),
+                            !cast<SchedRead>("ReadVMergeOp_" # mx # "_E" #sew));
+  defvar allReads = !if(isMaskedOrForceMasked,
+                        !listconcat([mergeOp], reads, [ReadVMask]),
+                        reads);
+  let SchedRW = !listconcat(writes, allReads);
 }
 
 // Common class of scheduling definitions for n-ary instructions.
 // The scheudling resources are relevant to LMUL and may be relevant to SEW.
 class SchedNary<string write, list<string> reads,
                 string mx, int sew = 0, bit forceMasked = 0>:
   SchedCommon<[!cast<SchedWrite>(
                  !if(sew, write # "_" # mx # "_E" # sew,
                           write # "_" # mx))],
               !foreach(read, reads,
                 !cast<SchedRead>(
                   !if(sew, read # "_" # mx # "_E" # sew,
                            read # "_" # mx))),
-              forceMasked>;
+              mx, sew, forceMasked>;
 
 // Classes with postfix "MC" are only used in MC layer.
 // For these classes, we assume that they are with the worst case costs and
 // `ReadVMask` is always needed (with some exceptions).
 
 // For instructions with no operand.
 class SchedNullary<string write, string mx, int sew = 0, bit forceMasked = 0>:
   SchedNary<write, [], mx, sew, forceMasked>;

 // For instructions with three operands.
 class SchedTernary<string write, string read0, string read1, string read2,
                    string mx, int sew = 0, bit forceMasked = 0>:
   SchedNary<write, [read0, read1, read2], mx, sew, forceMasked>;
 
 // For reduction instructions.
 class SchedReduction<string write, string read, string mx, int sew>:
   SchedCommon<[!cast<SchedWrite>(write # "_" # mx # "_E" # sew)],
-              !listsplat(!cast<SchedRead>(read), 3)>;
+              !listsplat(!cast<SchedRead>(read), 3), mx, sew>;
 class SchedReductionMC<string write, string readV, string readV0>:
   SchedCommon<[!cast<SchedWrite>(write # "_WorstCase")],
               [!cast<SchedRead>(readV), !cast<SchedRead>(readV0)],
               forceMasked=1>;
 
 // Whole Vector Register Move
 class VMVRSched<int n> : SchedCommon<
-  [!cast<SchedWrite>("WriteVMov" #n #"V")],
-  [!cast<SchedRead>("ReadVMov" #n #"V")]
+  [!cast<SchedWrite>("WriteVMov" # n # "V")],
+  [!cast<SchedRead>("ReadVMov" # n # "V")]
 >;
 
 // Vector Unit-Stride Loads and Stores
 class VLESched<string lmul, bit forceMasked = 0> : SchedCommon<
-  [!cast<SchedWrite>("WriteVLDE_" #lmul)],
-  [ReadVLDX], forceMasked
+  [!cast<SchedWrite>("WriteVLDE_" # lmul)],
+  [ReadVLDX], mx=lmul, forceMasked=forceMasked
 >;
 class VLESchedMC : VLESched<"WorstCase", forceMasked=1>;
 
 class VSESched<string lmul, bit forceMasked = 0> : SchedCommon<
-  [!cast<SchedWrite>("WriteVSTE_" #lmul)],
-  [!cast<SchedRead>("ReadVSTEV_" #lmul), ReadVSTX], forceMasked
+  [!cast<SchedWrite>("WriteVSTE_" # lmul)],
+  [!cast<SchedRead>("ReadVSTEV_" # lmul), ReadVSTX], mx=lmul,
+  forceMasked=forceMasked
 >;
 class VSESchedMC : VSESched<"WorstCase", forceMasked=1>;
 
 // Vector Strided Loads and Stores
 class VLSSched<int eew, string emul, bit forceMasked = 0> : SchedCommon<
-  [!cast<SchedWrite>("WriteVLDS" #eew #"_" #emul)],
-  [ReadVLDX, ReadVLDSX], forceMasked
+  [!cast<SchedWrite>("WriteVLDS" # eew # "_" # emul)],
+  [ReadVLDX, ReadVLDSX], emul, eew, forceMasked
 >;
 class VLSSchedMC<int eew> : VLSSched<eew, "WorstCase", forceMasked=1>;
 
 class VSSSched<int eew, string emul, bit forceMasked = 0> : SchedCommon<
-  [!cast<SchedWrite>("WriteVSTS" #eew #"_" #emul)],
-  [!cast<SchedRead>("ReadVSTS" #eew #"V_" #emul), ReadVSTX, ReadVSTSX],
-  forceMasked
+  [!cast<SchedWrite>("WriteVSTS" # eew # "_" # emul)],
+  [!cast<SchedRead>("ReadVSTS" # eew # "V_" # emul), ReadVSTX, ReadVSTSX],
+  emul, eew, forceMasked
 >;
 class VSSSchedMC<int eew> : VSSSched<eew, "WorstCase", forceMasked=1>;
 
 // Vector Indexed Loads and Stores
 class VLXSched<int dataEEW, string isOrdered, string dataEMUL, string idxEMUL,
                bit forceMasked = 0> : SchedCommon<
-  [!cast<SchedWrite>("WriteVLD" #isOrdered #"X" #dataEEW #"_" #dataEMUL)],
-  [ReadVLDX, !cast<SchedRead>("ReadVLD" #isOrdered #"XV_" #idxEMUL)],
-  forceMasked
+  [!cast<SchedWrite>("WriteVLD" # isOrdered # "X" # dataEEW # "_" # dataEMUL)],
+  [ReadVLDX, !cast<SchedRead>("ReadVLD" # isOrdered # "XV_" # idxEMUL)],
+  dataEMUL, dataEEW, forceMasked
 >;
 class VLXSchedMC<int dataEEW, string isOrdered>:
   VLXSched<dataEEW, isOrdered, "WorstCase", "WorstCase", forceMasked=1>;
 
 class VSXSched<int dataEEW, string isOrdered, string dataEMUL, string idxEMUL,
                bit forceMasked = 0> : SchedCommon<
-  [!cast<SchedWrite>("WriteVST" #isOrdered #"X" #dataEEW #"_" #dataEMUL)],
-  [!cast<SchedRead>("ReadVST" #isOrdered #"X" #dataEEW #"_" #dataEMUL),
-   ReadVSTX, !cast<SchedRead>("ReadVST" #isOrdered #"XV_" #idxEMUL)],
-  forceMasked
+  [!cast<SchedWrite>("WriteVST" # isOrdered # "X" # dataEEW # "_" # dataEMUL)],
+  [!cast<SchedRead>("ReadVST" # isOrdered #"X" # dataEEW # "_" # dataEMUL),
+   ReadVSTX, !cast<SchedRead>("ReadVST" # isOrdered # "XV_" # idxEMUL)],
+  dataEMUL, dataEEW, forceMasked
 >;
 class VSXSchedMC<int dataEEW, string isOrdered>:
   VSXSched<dataEEW, isOrdered, "WorstCase", "WorstCase", forceMasked=1>;
 
 // Unit-stride Fault-Only-First Loads
 class VLFSched<string lmul, bit forceMasked = 0> : SchedCommon<
-  [!cast<SchedWrite>("WriteVLDFF_" #lmul)],
-  [ReadVLDX], forceMasked
+  [!cast<SchedWrite>("WriteVLDFF_" # lmul)],
+  [ReadVLDX], mx=lmul, forceMasked=forceMasked
 >;
 class VLFSchedMC: VLFSched<"WorstCase", forceMasked=1>;
 
 // Unit-Stride Segment Loads and Stores
 class VLSEGSched<int nf, int eew, string emul, bit forceMasked = 0> : SchedCommon<
   [!cast<SchedWrite>("WriteVLSEG" #nf #"e" #eew #"_" #emul)],
-  [ReadVLDX], forceMasked
+  [ReadVLDX], emul, eew, forceMasked
 >;
 class VLSEGSchedMC<int nf, int eew> : VLSEGSched<nf, eew, "WorstCase",
                                                  forceMasked=1>;
 
 class VSSEGSched<int nf, int eew, string emul, bit forceMasked = 0> : SchedCommon<
-  [!cast<SchedWrite>("WriteVSSEG" #nf #"e" #eew #"_" #emul)],
-  [!cast<SchedRead>("ReadVSTEV_" #emul), ReadVSTX], forceMasked
+  [!cast<SchedWrite>("WriteVSSEG" # nf # "e" # eew # "_" # emul)],
+  [!cast<SchedRead>("ReadVSTEV_" #emul), ReadVSTX], emul, eew, forceMasked
 >;
 class VSSEGSchedMC<int nf, int eew> : VSSEGSched<nf, eew, "WorstCase",
                                                  forceMasked=1>;
 
 class VLSEGFFSched<int nf, int eew, string emul, bit forceMasked = 0> : SchedCommon<
-  [!cast<SchedWrite>("WriteVLSEGFF" #nf #"e" #eew #"_" #emul)],
-  [ReadVLDX], forceMasked
+  [!cast<SchedWrite>("WriteVLSEGFF" # nf # "e" # eew # "_" # emul)],
+  [ReadVLDX], emul, eew, forceMasked
 >;
 class VLSEGFFSchedMC<int nf, int eew> : VLSEGFFSched<nf, eew, "WorstCase",
                                                      forceMasked=1>;
 
 // Strided Segment Loads and Stores
 class VLSSEGSched<int nf, int eew, string emul, bit forceMasked = 0> : SchedCommon<
   [!cast<SchedWrite>("WriteVLSSEG" #nf #"e" #eew #"_" #emul)],
-  [ReadVLDX, ReadVLDSX], forceMasked
+  [ReadVLDX, ReadVLDSX], emul, eew, forceMasked
 >;
 class VLSSEGSchedMC<int nf, int eew> : VLSSEGSched<nf, eew, "WorstCase",
                                                    forceMasked=1>;
 
 class VSSSEGSched<int nf, int eew, string emul, bit forceMasked = 0> : SchedCommon<
   [!cast<SchedWrite>("WriteVSSSEG" #nf #"e" #eew #"_" #emul)],
   [!cast<SchedRead>("ReadVSTS" #eew #"V_" #emul),
-   ReadVSTX, ReadVSTSX], forceMasked
+   ReadVSTX, ReadVSTSX], emul, eew, forceMasked
 >;
 class VSSSEGSchedMC<int nf, int eew> : VSSSEGSched<nf, eew, "WorstCase",
                                                    forceMasked=1>;
 
 // Indexed Segment Loads and Stores
 class VLXSEGSched<int nf, int eew, string isOrdered, string emul,
                   bit forceMasked = 0> : SchedCommon<
   [!cast<SchedWrite>("WriteVL" #isOrdered #"XSEG" #nf #"e" #eew #"_" #emul)],
   [ReadVLDX, !cast<SchedRead>("ReadVLD" #isOrdered #"XV_" #emul)],
-  forceMasked
+  emul, eew, forceMasked
 >;
 class VLXSEGSchedMC<int nf, int eew, string isOrdered>:
   VLXSEGSched<nf, eew, isOrdered, "WorstCase", forceMasked=1>;
 
+// Passes sew=0 instead of eew=0 since this pseudo does not follow MX_E form.
 class VSXSEGSched<int nf, int eew, string isOrdered, string emul,
                   bit forceMasked = 0> : SchedCommon<
   [!cast<SchedWrite>("WriteVS" #isOrdered #"XSEG" #nf #"e" #eew #"_" #emul)],
   [!cast<SchedRead>("ReadVST" #isOrdered #"X" #eew #"_" #emul),
    ReadVSTX, !cast<SchedRead>("ReadVST" #isOrdered #"XV_" #emul)],
-  forceMasked
+  emul, sew=0, forceMasked=forceMasked
 >;
 class VSXSEGSchedMC<int nf, int eew, string isOrdered>:
   VSXSEGSched<nf, eew, isOrdered, "WorstCase", forceMasked=1>;
 
 //===----------------------------------------------------------------------===//
 // Instruction class templates
 //===----------------------------------------------------------------------===//
 

llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td

 }
 
 //===----------------------------------------------------------------------===//
 // Combination of instruction classes.
 // Use these multiclasses to define instructions more easily.
 //===----------------------------------------------------------------------===//
 multiclass VPseudoVMAQA_VV_VX {
   foreach m = MxListTHVdot in {
+    // TODO: Add Sched
     defm "" : VPseudoTernaryW_VV<m>;
     defm "" : VPseudoTernaryW_VX<m>;
   }
 }
 
 multiclass VPseudoVMAQA_VX {
   foreach m = MxListTHVdot in {
+    // TODO: Add Sched
     defm "" : VPseudoTernaryW_VX<m>;
   }
 }
 
 multiclass VPatTernaryVMAQA_VV<string intrinsic, string instruction,
                                list<VTypeInfoToWide> vtilist> {
   foreach vtiToWti = vtilist in {
     defvar vti = vtiToWti.Vti;

llvm/lib/Target/RISCV/RISCVSchedSiFive7.td

 // LMUL Aware
 def : ReadAdvance<ReadVMov1V, 0>;
 def : ReadAdvance<ReadVMov2V, 0>;
 def : ReadAdvance<ReadVMov4V, 0>;
 def : ReadAdvance<ReadVMov8V, 0>;
 
 // Others
 def : ReadAdvance<ReadVMask, 0>;
+def : ReadAdvance<ReadVMergeOp_WorstCase, 0>;
+foreach mx = SchedMxList in {
+  def : ReadAdvance<!cast<SchedRead>("ReadVMergeOp_" # mx), 0>;
+  foreach sew = SchedSEWSet<mx>.val in
+    def : ReadAdvance<!cast<SchedRead>("ReadVMergeOp_" # mx  # "_E" # sew), 0>;
+}
 
 //===----------------------------------------------------------------------===//
 // Unsupported extensions
 defm : UnsupportedSchedZbc;
 defm : UnsupportedSchedZbkb;
 defm : UnsupportedSchedZbkx;
 defm : UnsupportedSchedZfa;
 }

llvm/lib/Target/RISCV/RISCVScheduleV.td

 // These are already LMUL aware
 def ReadVMov1V        : SchedRead;
 def ReadVMov2V        : SchedRead;
 def ReadVMov4V        : SchedRead;
 def ReadVMov8V        : SchedRead;
 
 // Others
 def ReadVMask         : SchedRead;
+def ReadVMergeOp_WorstCase : SchedRead;
+foreach mx = SchedMxList in {
+  def ReadVMergeOp_ # mx : SchedRead;
+  foreach sew = SchedSEWSet<mx>.val in
+    def ReadVMergeOp_ # mx  # "_E" # sew : SchedRead;
+}
 
 //===----------------------------------------------------------------------===//
 /// Define default scheduler resources for V.
 
 multiclass UnsupportedSchedV {
 let Unsupported = true in {
 
 // 3.6 Vector Byte Length vlenb

 // These are already LMUL aware
 def : ReadAdvance<ReadVMov1V, 0>;
 def : ReadAdvance<ReadVMov2V, 0>;
 def : ReadAdvance<ReadVMov4V, 0>;
 def : ReadAdvance<ReadVMov8V, 0>;
 
 // Others
 def : ReadAdvance<ReadVMask, 0>;
+def : ReadAdvance<ReadVMergeOp_WorstCase, 0>;
+foreach mx = SchedMxList in {
+  def : ReadAdvance<!cast<SchedRead>("ReadVMergeOp_" # mx), 0>;
+  foreach sew = SchedSEWSet<mx>.val in
+    def : ReadAdvance<!cast<SchedRead>("ReadVMergeOp_" # mx  # "_E" # sew), 0>;
+}
 
 } // Unsupported
 } // UnsupportedSchedV