Add the scheduling resources for the V extension instructions.
Details
Diff Detail
Event Timeline
I'm curious how instruction scheduling works if you don't add those resource models into the PseudoRVV instructions?
I've given it a quick once-over. Apologies if I've missed the mark in some of my comments; there might be reasons for some of them that I'm unaware of.
In general, I'm not seeing resources for mask reads? Should those be modeled?
llvm/lib/Target/RISCV/RISCVInstrInfoV.td | ||
---|---|---|
353 | Should this use ReadVIALUX? | |
438 | Is this missing a read of an X resource? | |
484 | Should this be WriteVFDivF? | |
739 | ReadVNShiftV instead of ReadVShiftV? | |
741 | ReadVNShiftV instead of ReadVShiftV? | |
757 | Missing ReadVICmpX? |
llvm/lib/Target/RISCV/RISCVInstrInfoV.td | ||
---|---|---|
1017 | Feels like there's too many read resources: VMV_V_V only has one operand? | |
1021 | Again, too many read resources? VMV_V_X only has one operand. | |
1025 | Too many read resources? | |
1146 | Unsure here, but since this reads a scalar should it read something like ReadVFMergeF? | |
1153 | Too many read operands? Should it read only something like ReadVFMovF? | |
1321 | Missing read resource? | |
1362 | Do all nfs 1,2,4,8 have the same 1V resource? |
llvm/lib/Target/RISCV/RISCVSchedRocket.td | ||
---|---|---|
19 | Does anything fail without this change? I assumed the UnsupportedFeatures list covered this. |
llvm/lib/Target/RISCV/RISCV.td | ||
---|---|---|
232 | Other targets do it here, but I lean towards your suggestion. |
llvm/lib/Target/RISCV/RISCVInstrInfoV.td | ||
---|---|---|
1178–1185 | It might not really be important but it's not immediately obvious to me what some of these abbreviations signify. VNCVTF narrow-converts int to float but VNCVTD narrow-converts float to float? Why F and D? | |
1303 | How come ReadDefault? Also seems out of sync with VFMV_S_F below. |
Adding those "sched" information into base RVV instructions seems redundant? The scheduler works on PseudoRVV instructions.
SchedReadWrite needs to consider pseudo instructions with LMUL? Their latency should be different.
Not redundant, as some tools do use the information at this level, e.g., llvm-mca.
Moreover, this patch captures the resources in a way that it makes it easier to add latency information to specific targets later.
SchedReadWrite needs to consider pseudo instructions with LMUL? Their latency should be different.
And SEW. I'm working on this feature, of which this patch introduces the preliminary information.
llvm/lib/Target/RISCV/RISCVInstrInfoV.td | ||
---|---|---|
810 | Do the saturating instructions need resources for VXSAT/VXRM? |
llvm/lib/Target/RISCV/RISCVInstrInfoV.td | ||
---|---|---|
106 | No scheduling resource for store value? |
llvm/lib/Target/RISCV/RISCVInstrInfoV.td | ||
---|---|---|
810 | It doesn't seem to me that sideband registers needs must be modeled for scheduling. |
llvm/lib/Target/RISCV/RISCVInstrInfoV.td | ||
---|---|---|
794 | Should ReadVLDX be ReadVSTX for VSE1_V? | |
1264 | Should the last be ReadVMask? | |
1270 | Should the last be ReadVMask? | |
1293 | Why is this ReadVMIdxV and not ReadVMask? | |
llvm/lib/Target/RISCV/RISCVScheduleV.td | ||
91 | There are no X or I forms for extensions are there? | |
314 | Is ReadVExtX used? |
llvm/lib/Target/RISCV/RISCVInstrInfoV.td | ||
---|---|---|
1178–1185 | vfncvt.f.x.w vd, vs2, vm # Convert double-width signed integer to float. vfncvt.f.f.w vd, vs2, vm # Convert double-width float to single-width float. These two are both to convert to single-width float. Maybe use DI, IF and DF to distinguish them? | |
1193–1200 | Why the suffix changed to _IV_V? It is belong to OPMVV format. | |
1218–1221 | Why is there VREDO_FV_V, but no VWREDO_FV_V? | |
1238–1245 | The suffix could keep the same as before. |
I think all my comments have been addressed. I think one suggestion from @HsiangKai is still open. @frasercrmck what do you think?
llvm/lib/Target/RISCV/RISCVInstrInfoV.td | ||
---|---|---|
1178–1185 | vncvt.f.f.w is also used for float to half. So using something that stands for "double" and "float" could be misleading. |
llvm/lib/Target/RISCV/RISCVInstrInfoV.td | ||
---|---|---|
1178–1185 | I agree. It is misleading. How about use F_X and F_F just like the instruction name. |
llvm/lib/Target/RISCV/RISCVInstrInfoV.td | ||
---|---|---|
547 | I don't know either. It would probably be used with ReadAdvance. | |
1178–1185 | The general scheme above is V{,W,N}CVT{I,F}_{I,F}V_VS2, where {,W,N} denotes the width of the result, whether the same, wider or narrower, and {I,F}, the type of the result, in the first instance, or of the argument, in the second instance, integer or FP. |
llvm/lib/Target/RISCV/RISCVInstrInfoV.td | ||
---|---|---|
1178–1185 | LGTM. |
I'll formally LGTM given people seem happy overall.
llvm/lib/Target/RISCV/RISCVInstrInfoV.td | ||
---|---|---|
547 | Yeah possibly. We can cross that bridge when we come to it, I think. |
Hi, I am wondering is there any patch to construct Write/Read Sched for pseudo instructions with every different LMUL now? @evandro
Maybe include this from RISCVSchedule.td the way RISCVInstrInfo.td includes RISCVInstrInfoV.td?