Add scheduling information for vector extension in SiFive7,
while using new LMUL & SEW scheduling constructs.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/RISCV/RISCVProcessors.td | ||
---|---|---|
169 | 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
llvm/lib/Target/RISCV/RISCVSchedSiFive7.td | ||
---|---|---|
112 | There's an extra space before VLUpperBound here |
LGTM, thought please wait for other review feedback to settle.
I am very very happy to have this upstream, thank you!
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? |
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). |
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. |
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. |
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). |
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!
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.