This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][NFC] Replace the pseudos for instructions that depend on lmul with variants that encode the SEW into the name
ClosedPublic

Authored by nitinjohnraj on Feb 27 2023, 3:26 PM.

Details

Summary
  • vcompress
  • vrgather.vv/vrgatherei16.vv (vrgather.vi/vx are fine)
  • vdiv/vdivu/vfdiv
  • vsqr

These instructions depend on vl, so we will create pseudos for these instructions that depend on SEW, since VLUpperBound = LMUL * VLEN / SEW. This will, in the future, help us reason about them more finely in the scheduler model.

These instructions also depend on vl, and will be dealt with in the next revision:

  • strided and indexed load/stores.
  • segment load/stores
  • v(w)(f)red*

Diff Detail

Event Timeline

nitinjohnraj created this revision.Feb 27 2023, 3:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2023, 3:26 PM
nitinjohnraj requested review of this revision.Feb 27 2023, 3:26 PM
nitinjohnraj retitled this revision from [RISCV][NFC] Added possible SEWs associated with a given LMUL to [RISCV][NFC] Expand pseudos with sew for instructions that depend on lmul.Feb 27 2023, 3:35 PM
nitinjohnraj edited the summary of this revision. (Show Details)
nitinjohnraj added a reviewer: craig.topper.
nitinjohnraj edited the summary of this revision. (Show Details)Feb 27 2023, 3:42 PM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
1919

If we're going to use suffix, should we use it in WriteVCompressV_MX_E and ReadVCompressV_MX_E too?

2041–2061

WriteVGatherV_MX_E and ReadVGatherV_MX_E dont depend on sew from EEWList. Can we pull out the loop over sews from SchedSEWSet<mx>.val to simplify?

2336

Use suffix in WriteVFSqrtV_MX and ReadVFSqrtV_MX. Also need to add _E to WriteVFSqrtV_MX and ReadVFSqrtV_MX

2353

Where did you land on making this SEW aware?

2469–2470

nit: define sews closer to where you use it?

2470

Why did we change ReadVGatherV to ReadVGatherI?

llvm/lib/Target/RISCV/RISCVScheduleV.td
24

Why don't we make this case SEW aware?

46

Why don't we make this case SEW aware?

50

Why don't we make this case SEW aware?

61

Why don't we make this case SEW aware?

Just a glancing review but, to me, "expand pseudos" has a more immediate connotation of the many pseudo expansion passes (e.g., RISCVExpandPseudoInsts) than "expanding the set of pseudos" as you seem to intend it.

nitinjohnraj edited the summary of this revision. (Show Details)Feb 28 2023, 10:43 AM
nitinjohnraj retitled this revision from [RISCV][NFC] Expand pseudos with sew for instructions that depend on lmul to [RISCV][NFC] Replace the pseudos for instructions that depend on lmul with variants that encode the SEW into the name .Feb 28 2023, 2:46 PM

Just a glancing review but, to me, "expand pseudos" has a more immediate connotation of the many pseudo expansion passes (e.g., RISCVExpandPseudoInsts) than "expanding the set of pseudos" as you seem to intend it.

Thanks for the comment, that makes sense. :)

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

Good point, yeah.

2041–2061

Thanks for the review! We can certainly do that, but does it really simplify the code? It's a nested loop in either case, so I'm not clear which version performs better/looks simpler.

2336

Thanks, will do!

2469–2470

Appreciated, I agree with that nit, seems better for clarity.

2470

After discussion with @craig.topper, we did this because otherwise we'd need to maintain both SEW-aware and SEW-unaware versions of ReadVGatherV:

  1. The SEW-aware version on line 2481 for the _VV binary, which is SEW-aware itself
  2. The SEW-unaware version for the _VI binary, which is not SEW aware

This change helps us generate only the SEW-aware ReadVGatherV SchedReads. I'd be glad to talk about this more if you think we should approach this differently though.

2477–2478

I feel particularly uneasy about this change. I could leave it as is, or I could create ReadVGatherX_MX with no SEW in RISCVScheduleV.td.

llvm/lib/Target/RISCV/RISCVScheduleV.td
24

If I remember correctly, that was causing some other files to complain and I put this in as

craig.topper added inline comments.Feb 28 2023, 3:11 PM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
2470

We should move this part to a separate patch. vrgather.vi, vrgather.vx, vrgather.vv are all sort of different.

Let's consider a uarch that splits a LMUL>1 into multiple subvectors acrosss multiple uops:
vrgather.vi knows at decode time which subvector of the full source is needed for the destination subvectors
vrgather.vx doesn't know until the scalar instruction executes which subvector of the full source is needed. But only one subvector will be needed.
vrgather.vv every subvector of the destination can be dependent on multiple subvectors of the source.

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
2041–2061

You are right. Good as you have it :)

llvm/lib/Target/RISCV/RISCVScheduleV.td
24

I think this should be sew aware here and below and we need to fix the other files that start complaining.

nitinjohnraj marked 8 inline comments as done and an inline comment as not done.
nitinjohnraj retitled this revision from [RISCV][NFC] Replace the pseudos for instructions that depend on lmul with variants that encode the SEW into the name to [RISCV][NFC] Replace the pseudos for instructions that depend on lmul with variants that encode the SEW into the name.

Made the SchedReads and SchedWrites sew-aware

A lot of the content from your original patch is no longer showing in this review. For example, SchedSEWSet is no longer created in this diff.

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

I think the answer is no.

nitinjohnraj marked 2 inline comments as done.Mar 14 2023, 5:29 PM

Created a diff from a commit range this time

michaelmaitland requested changes to this revision.Mar 15 2023, 10:26 AM
michaelmaitland added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
2336

defvar WriteVFSqrtV_MX_E and defvar WriteVFSqrtV_MX_E

2353

If this does not need to be SEW aware, should we remove the TODO?

This revision now requires changes to proceed.Mar 15 2023, 10:26 AM

Diff updated to add another commit that was missed last time (Sorry, never used commit ranges with arcanist before.)

nitinjohnraj marked 6 inline comments as done.

Refactoring and removing comments

nitinjohnraj marked an inline comment as not done.Mar 15 2023, 10:43 AM
nitinjohnraj added inline comments.
llvm/lib/Target/RISCV/RISCVScheduleV.td
50

Did we want to make UpperBound SEW-aware, or handle it in a different way? IIRC we discussed using it as a defined variable?

71

Same as line 62

80

Same as line 62

90

Same as line 62

nitinjohnraj marked an inline comment as not done.Mar 15 2023, 1:02 PM
nitinjohnraj marked an inline comment as not done.
llvm/lib/Target/RISCV/RISCVScheduleV.td
50

I think the best solution for now is to use the largest LMUL and the largest SEW for that given LMUL as the UpperBound SchedWrite. This assumes that the largest LMUL and SEW has the largest latency and uses the most resource cycles. This assumption may not always true, but I think is a good starting point.

craig.topper added inline comments.Mar 15 2023, 1:09 PM
llvm/lib/Target/RISCV/RISCVScheduleV.td
50

If the worst case is the number of elements rather than the size of elements you'd want to use smallest sew.

Attempting to fix the diff

michaelmaitland accepted this revision.Mar 21 2023, 10:28 AM

LGTM. As we discussed offline, today UpperBound refers to the largest LMUL supported. This should be changed in a future patch. I propose that it should be removed from MxList and its meaning should be tied the SchedWrite with the worst case behaviour. Now that we are supporting SchedWrite by LMUL + SEW or just LMUL depending on the instruction, the specific scheduler model can decide which LMUL or LMUL + SEW corresponds to the worst case behaviour. This means no assumption is made on whether the Largest LMUL or smallest SEW behaves the worst.

This revision is now accepted and ready to land.Mar 21 2023, 10:28 AM

Is this still an open issue? I think you may have committed all the parts.

evandro removed a subscriber: evandro.Jun 27 2023, 9:41 AM

These changes have been committed.