Page MenuHomePhabricator

[mlir][RISCVV][RFC] Initial RISCVV Dialect
Needs ReviewPublic

Authored by zhanghb97 on Aug 23 2021, 3:05 AM.

Details

Summary

This is the inital patch for RISC-V vector extension (RVV) dialect. This patch include:

  • RVV Dialect Definition
    • RVV Scalable Vector Type
    • RVV Operations
    • RVV Intrinsic Operations
  • Translation from RVV Dialect to LLVM Dialect

Diff Detail

Event Timeline

zhanghb97 created this revision.Aug 23 2021, 3:05 AM
zhanghb97 requested review of this revision.Aug 23 2021, 3:05 AM

FYI - I wrote the RFC in the LLVM discourse.

some initial comments, we are also having a discussion at discourse in the RFC thread

mlir/include/mlir/Dialect/RVV/RVV.td
27–29 ↗(On Diff #368053)

This is the part that will show up on the public doc website. Perhaps you can give a bit more background here, and link to the proper places to find out more onthe RISC-V Vector ops you introduce here.

39 ↗(On Diff #368053)

this is copy and past from ArmSVE. We should really try to lift this to a shared definition at the proper place

87 ↗(On Diff #368053)

empty line after this separator for consistency

mlir/lib/Dialect/RVV/Transforms/LegalizeForLLVMExport.cpp
150 ↗(On Diff #368053)

period at tend

mlir/test/Dialect/RVV/roundtrip.mlir
3 ↗(On Diff #368053)

here and below, use

CHECK-LABEL:

nit on the naming, rvv feels a bit too general. I'd rather go with a longer name that spells out riscv: riscvv or something related.

Joejiong added inline comments.Aug 26 2021, 3:44 AM
mlir/lib/Dialect/RVV/Transforms/LegalizeForLLVMExport.cpp
157 ↗(On Diff #368053)

double // added

zhanghb97 updated this revision to Diff 369225.Aug 27 2021, 8:07 PM
zhanghb97 marked 4 inline comments as done.
  • Add more background about RVV and introduction to operations in this dialect.
  • Add CHECK-LABELs for the round trip test.
  • Fix some format issues.

nit on the naming, rvv feels a bit too general. I'd rather go with a longer name that spells out riscv: riscvv or something related.

Some communities tend to use RVV as the name. For example, the rvv-intrinsic-doc and OpenCV wide universal intrinsic definition: intrin_rvv.hpp. So I also use the RVV on the MLIR side at the very beginning.

If the "rvv/RVV" is too general, and to be consistent with MLIR naming convention (arm_sve/ArmSVE, x86vector/X86Vector, etc.), how about using :

  • "riscv_vector" for the namespace
  • "RISCVVector" for the file name, folder name, etc.

What do you think about these names?

mlir/include/mlir/Dialect/RVV/RVV.td
39 ↗(On Diff #368053)

I think we can make the vector type support both static and scalable versions, for example:

  • static version for SIMD architecture vector<4xi64>
  • scalable version for vector architecture vector<?x4xi64>

And different ISAs can have different semantics. What do you think about this method?
I will spend some time figuring out how to implement this idea.

mlir/lib/Dialect/RVV/Transforms/LegalizeForLLVMExport.cpp
150 ↗(On Diff #368053)

Sorry, I don’t understand what "period at tend" means.

zhanghb97 added a comment.EditedAug 27 2021, 8:14 PM

As for the integration test, I currently encounter some problems, and I will continue to find some solutions. At the same time, I will try to move the scalable vector type into the vector dialect in the next step.

And for the names, if the "rvv" is to general, we can use the following names:

  • "riscv_vector" for the namespace
  • "RISCVVector" for the file name, folder name, etc.

This needs further confirmation.

zhanghb97 marked an inline comment as not done.Aug 27 2021, 8:15 PM
Matt added a subscriber: Matt.Aug 30 2021, 2:04 AM
Joejiong added inline comments.Aug 30 2021, 2:39 AM
mlir/include/mlir/Dialect/RVV/RVV.td
29 ↗(On Diff #369225)

a small typo: pleas -> please

zhanghb97 updated this revision to Diff 369699.Aug 31 2021, 7:51 AM
  • Fix typo.
  • Lift the scalable vector type to vector dialect. The SVE and RVV can share the definition now.
zhanghb97 marked an inline comment as done.Aug 31 2021, 8:02 AM
zhanghb97 added inline comments.
mlir/include/mlir/Dialect/RVV/RVV.td
39 ↗(On Diff #368053)

After further thinking, I think it is better for SVE and RVV to share the current scalable vector type definition. The unified scalable vector type (vector<?x4xi64> or something else) needs more exploration in the future. And I have lifted the definition to make the scalable vector type a shared version.

zhanghb97 retitled this revision from [mlir][RVV][RFC] Initial RVV Dialect to [mlir][RVV][RFC][WIP] Initial RVV Dialect.Aug 31 2021, 8:03 AM

For the next step, I will try to give better semantics to the type of RVV:

  • let the vector type directly map LMUL and SEW.
  • add a type conversion when lower the RVV operation to RVV intrinsic operation.
jurahul added inline comments.Aug 31 2021, 9:19 AM
mlir/lib/Dialect/RVV/Transforms/LegalizeForLLVMExport.cpp
150 ↗(On Diff #368053)

period at end. https://llvm.org/docs/CodingStandards.html#commenting ("using proper capitalization, punctuation, etc").

meshtag added a subscriber: meshtag.Sep 3 2021, 4:15 AM
zhanghb97 updated this revision to Diff 372431.Sep 14 2021, 1:26 AM
  • Naming (RVV -> RISCVV)
  • Scalable Vector Type (Common Scalable Vector Type + Separate RISC-V Scalable Vector Type)
    • Lift Scalable Vector Type to Vector Dialect
    • Define RVV LMUL Type, RVV Mask Type, and RVV Vector Type
    • Implement Type Mapping Process
zhanghb97 added inline comments.Sep 14 2021, 1:28 AM
mlir/lib/Dialect/RVV/Transforms/LegalizeForLLVMExport.cpp
150 ↗(On Diff #368053)

Thanks for informing this!

zhanghb97 retitled this revision from [mlir][RVV][RFC][WIP] Initial RVV Dialect to [mlir][RISCVV][RFC] Initial RISCVV Dialect.Sep 14 2021, 1:29 AM

As for the integration test, I currently encounter some problems, and I will continue to find some solutions.

Have you made more progress on this? I realize this is a bit of a hazzle, but leaving it as a TODO has a very high risk of not being done at all... :-(

Have you made more progress on this? I realize this is a bit of a hazzle, but leaving it as a TODO has a very high risk of not being done at all... :-(

I also realize the risk of not having the integration test. If LLVM IR intrinsics are changed (e.g. add a argument like this patch), the unit tests on MLIR will not be affected, but there will be problems when translating to LLVM IR.

I am trying to figure out why the cross-compiled lli and mlir-cpu-runner fail to identify the target with the QEMU, And I will also use a RISC-V machine to have a try.

zhanghb97 updated this revision to Diff 377425.EditedTue, Oct 5, 9:31 PM

WIP: integration test