This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add support for V extension in SiFive7
ClosedPublic

Authored by michaelmaitland on Apr 28 2023, 4:09 PM.

Details

Summary

Add scheduling information for vector extension in SiFive7,
while using new LMUL & SEW scheduling constructs.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2023, 4:09 PM
michaelmaitland requested review of this revision.Apr 28 2023, 4:09 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 28 2023, 4:09 PM
michaelmaitland retitled this revision from Add sifive-x280 processor and support V extenstion in SiFive7 to [RISCV] Add sifive-x280 processor and support V extenstion in SiFive7.Apr 28 2023, 4:10 PM

Remove zfh from x280 check in clang/test/Driver/riscv-cpus.c

craig.topper retitled this revision from [RISCV] Add sifive-x280 processor and support V extenstion in SiFive7 to [RISCV] Add sifive-x280 processor and support V extension in SiFive7.May 1 2023, 9:52 PM
craig.topper added inline comments.
llvm/lib/Target/RISCV/RISCVProcessors.td
169 ↗(On Diff #518086)

I would prefer that we add sifive-x280 in a single patch with all of its features. Currently zba/zbb are in a different patch. I don't think there's any restriction about adding a feature to a CPU without it being present in the scheduler model.

All CPU name additions should be mentioned in the RISC-V section of llvm/docs/ReleaseNotes.rst

Split adding sifive-x280 and vector model between this patch and https://reviews.llvm.org/D149710

michaelmaitland retitled this revision from [RISCV] Add sifive-x280 processor and support V extension in SiFive7 to [RISCV] Add support for V extenstion in SiFive7.May 2 2023, 5:51 PM
michaelmaitland edited the summary of this revision. (Show Details)
craig.topper retitled this revision from [RISCV] Add support for V extenstion in SiFive7 to [RISCV] Add support for V extension in SiFive7.May 2 2023, 5:54 PM
michalt added a subscriber: michalt.May 5 2023, 9:55 AM
craig.topper added inline comments.May 8 2023, 6:26 PM
llvm/lib/Target/RISCV/RISCVSchedSiFive7.td
112

There's an extra space before VLUpperBound here

llvm/lib/Target/RISCV/RISCVSchedSiFive7.td
15

I think I have fixed the issue that defar can't refer to template arguments in D148197. So LMUL, SSEW and other fields can be replaced with defvars.

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

So, are we going to discard LMULXXXImpl below?

reames accepted this revision.May 9 2023, 7:59 AM

LGTM, thought please wait for other review feedback to settle.

I am very very happy to have this upstream, thank you!

This revision is now accepted and ready to land.May 9 2023, 7:59 AM
michaelmaitland marked an inline comment as done.

Remove extra space before VLUpperBound.

llvm/lib/Target/RISCV/RISCVSchedSiFive7.td
15

A defvar statement defines a global variable. SiFive7IsWorstCaseMX<mx, SchedMxList>.c becomes invalid syntax since c is not a member of SiFive7IsWorstCaseMX now that c is global. If we did use defvar here and gave c a more unique name so that it could fit in the global space, then we would also need to complicate it by adding a MXxxx_MxListxxx suffix for all mx and SchedMxList pairs.

I think we'd like to keep these as Type Iden = Value;. What do you think?

pcwang-thead accepted this revision.May 10 2023, 1:23 AM

LGTM.
Though I don't like the way that we need to loop MxList again when defining scheduling model, I think this can be a good practice to define RVV scheduling model in current TableGen's grammar.

llvm/lib/Target/RISCV/RISCVSchedSiFive7.td
15

I mean:

class SiFive7IsWorstCaseMX<string mx, list<string> MxList> {
  defvar LLMUL = LargestLMUL<MxList>.r;
  bit c = !eq(mx, LLMUL);
}

LLMUL can be a local variable so that there is only one field c(which is the result of this class-based subroutine). We can keep only one field as result and change other fields to local variables. In this way, we know that SiFive7IsWorstCaseMX is a subroutine conventionally and can be replace with function in the future(if my patches is merged).

michaelmaitland marked 2 inline comments as done.

Use defvar in subroutines.

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

I will use LMULXXXImpl in a follow up NFC patch. This code below comes from before LMULXXXImpl was introduced.

This revision was landed with ongoing or failed builds.May 10 2023, 10:34 AM
This revision was automatically updated to reflect the committed changes.
llvm/lib/Target/RISCV/RISCVScheduleV.td
41

Can you please provide an example of how you intend on this being used today? I think it depends on https://reviews.llvm.org/D146198 since if we use LMULXXXImpl today, there is no way to differentiate behavior based on LMUL.

evandro removed a subscriber: evandro.May 23 2023, 3:04 PM
llvm/lib/Target/RISCV/RISCVScheduleV.td
41

An example is the old version of this patch(https://reviews.llvm.org/D146198?vs=on&id=509607#toc) and the comment(https://reviews.llvm.org/D146198?vs=on&id=509607#4232814).
Maybe you can wait for the approval of these patches if we're gonna change it anyway. So I think there is no need to change it currently. :-)

Let's go ahead and land this as is, we can rework the stylistic pieces once the linked patches land.

michaelmaitland added a comment.EditedMay 25 2023, 1:32 PM

Let's go ahead and land this as is, we can rework the stylistic pieces once the linked patches land.

This has landed in https://reviews.llvm.org/rG1a855819a87f426bdbd83c815fa47ca01fdf928f. I raised my question regarding examples in my previous comment on this post because I am starting to rework the stylistic concerns raised by @pcwang-thead. However, I will wait until Wang's patches land before making any modifications to style.

Let's go ahead and land this as is, we can rework the stylistic pieces once the linked patches land.

This has landed in https://reviews.llvm.org/rG1a855819a87f426bdbd83c815fa47ca01fdf928f. I raised my question regarding examples in my previous comment on this post because I am starting to rework the stylistic concerns raised by @pcwang-thead. However, I will wait until Wang's patches land before making any modifications to style.

My apologies, I read right past the commit in the history. Oops. Sorry for the noise!