This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add scheduling resources for V
ClosedPublic

Authored by evandro on Mar 4 2021, 7:49 PM.

Details

Summary

Add the scheduling resources for the V extension instructions.

Diff Detail

Event Timeline

evandro created this revision.Mar 4 2021, 7:49 PM
evandro requested review of this revision.Mar 4 2021, 7:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2021, 7:49 PM

I'm curious about why there is no any load/store scheduling resources?

I'm curious about why there is no any load/store scheduling resources?

That's for another patch.

HsiangKai edited reviewers, added: HsiangKai; removed: Hsiang-Kai.Mar 7 2021, 9:43 PM

I'm curious how instruction scheduling works if you don't add those resource models into the PseudoRVV instructions?

I'm curious how instruction scheduling works if you don't add those resource models into the PseudoRVV instructions?

Firstly, this patch defines the resources. I'll follow it up with another patch.

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?

frasercrmck added inline comments.Mar 18 2021, 3:06 AM
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?

craig.topper added inline comments.Mar 20 2021, 11:46 PM
llvm/lib/Target/RISCV/RISCVSchedRocket.td
19

Does anything fail without this change? I assumed the UnsupportedFeatures list covered this.

evandro marked 11 inline comments as done.Mar 22 2021, 12:09 PM
evandro added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
1321

I'll have to look at the specifics of the vd_wb and vd operands.

1362

No, this is scrap.

llvm/lib/Target/RISCV/RISCVSchedRocket.td
19

This is scrap. Actually, I expected to need the unsupported classes list with this. Alas, it seems to be necessary.

evandro updated this revision to Diff 332392.Mar 22 2021, 12:12 PM

Add the mask operand.

evandro updated this revision to Diff 332399.Mar 22 2021, 12:41 PM
evandro marked an inline comment as done.
Harbormaster completed remote builds in B95070: Diff 332399.
evandro updated this revision to Diff 332432.Mar 22 2021, 2:43 PM
evandro marked an inline comment as done.
craig.topper added inline comments.Mar 22 2021, 4:05 PM
llvm/lib/Target/RISCV/RISCV.td
232

Maybe include this from RISCVSchedule.td the way RISCVInstrInfo.td includes RISCVInstrInfoV.td?

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

Do we need a Read for V0?

Max.Ma added a subscriber: Max.Ma.Mar 22 2021, 5:21 PM
evandro marked 3 inline comments as done.Mar 24 2021, 9:19 AM
evandro added inline comments.
llvm/lib/Target/RISCV/RISCV.td
232

Other targets do it here, but I lean towards your suggestion.

evandro updated this revision to Diff 333023.Mar 24 2021, 9:36 AM
evandro marked an inline comment as done.
evandro marked 2 inline comments as not done.Mar 24 2021, 10:17 AM
frasercrmck added inline comments.Mar 25 2021, 3:09 AM
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.

evandro added inline comments.Mar 26 2021, 2:13 PM
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
1178–1185

Narrowing conversion from double to float or widening from float to double.

1303

I'd like to revisit how these instruction were defined in the near future.

evandro updated this revision to Diff 333633.Mar 26 2021, 2:35 PM

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.

Adding those "sched" information into base RVV instructions seems redundant? The scheduler works on PseudoRVV instructions.

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.

evandro updated this revision to Diff 361486.Jul 24 2021, 8:30 PM

Add the scheduling resources for the V extension loads and stores.

frasercrmck added inline comments.Jul 26 2021, 4:07 AM
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
810

Do the saturating instructions need resources for VXSAT/VXRM?

HsiangKai added inline comments.Jul 26 2021, 7:20 AM
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
106

No scheduling resource for store value?

You may need to rebase on main branch.

evandro marked an inline comment as done.Jul 26 2021, 8:30 PM
evandro added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
810

It doesn't seem to me that sideband registers needs must be modeled for scheduling.

evandro updated this revision to Diff 361892.Jul 26 2021, 8:45 PM
craig.topper added inline comments.Jul 27 2021, 3:46 PM
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?

evandro updated this revision to Diff 362260.Jul 27 2021, 6:50 PM
evandro marked 6 inline comments as done.
HsiangKai added inline comments.Jul 28 2021, 2:54 AM
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.

evandro marked 3 inline comments as done.Jul 28 2021, 4:33 PM
evandro updated this revision to Diff 362584.Jul 28 2021, 4:59 PM

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.

HsiangKai added inline comments.Aug 2 2021, 7:26 AM
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.
Besides the naming issue, the patch looks good to me.

LGTM too, thanks @evandro! Just one last question I was wondering about regarding reductions.

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

I dunno if the first source of a reduction is likely to benefit from a different scheduling type than the second, since it's only reading the first element?

evandro updated this revision to Diff 363554.Aug 2 2021, 1:02 PM
evandro added inline comments.Aug 2 2021, 1:10 PM
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.

evandro updated this revision to Diff 363560.Aug 2 2021, 1:29 PM
evandro marked an inline comment as done.
HsiangKai added inline comments.Aug 3 2021, 1:08 AM
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
1178–1185

LGTM.

frasercrmck accepted this revision.Aug 3 2021, 1:47 AM

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.

This revision is now accepted and ready to land.Aug 3 2021, 1:47 AM
This revision was automatically updated to reflect the committed changes.

Adding those "sched" information into base RVV instructions seems redundant? The scheduler works on PseudoRVV instructions.

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.

Hi, I am wondering is there any patch to construct Write/Read Sched for pseudo instructions with every different LMUL now? @evandro