This patch adds the Zvfhmin extension for clang.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Don’t you need to make f16 vectors legal types in the backend?
And you need to disable intrinsics for instructions that aren’t supported by Zfhmin. Like f16 vector fadd
And you also need to make the backend only allow f16 vector operations that are supported with Zfhmin.
To enable specific EEW for specific insturction in instruction selection, I will create some parent revisions. Here is the first one. https://reviews.llvm.org/D150550
@michaelmaitland was also going to be working on supporting Zvfhmin for SiFive. Maybe we can split up and share some of the work?
@michaelmaitland , I update this revision with my local branch. May you have a look and see what missed compared with you version?
clang/include/clang/Basic/riscv_vector.td | ||
---|---|---|
2273–2279 | In general, I believe that vfwcvt_f_f_v and vfncvt_f_f_w do not require Zvfhmin or Zvfh. The only time that these intrinsics require Zvfhmin or Zvfh is when the operands to these intrinsics have EEW=16. | |
clang/lib/Sema/Sema.cpp | ||
2049 ↗ | (On Diff #523696) | Should this output zvfhmin instead of zvfh when experimental-zvfhmin feature is not included? |
clang/lib/Sema/Sema.cpp | ||
---|---|---|
2049 ↗ | (On Diff #523696) | Or maybe it makes more sense to change "zvfh" to "zvfh or zvfhmin"? |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
1018 ↗ | (On Diff #523696) | This needs to be below By default everything must be expanded. |
2096 ↗ | (On Diff #523696) | Is this correct? This function is called by RISCVTargetTransformInfo::isLegalToVectorizeReduction, isLegalMaskedLoadStore, isLegalMaskedGatherScatter. |
llvm/lib/Target/RISCV/RISCVSubtarget.h | ||
162 ↗ | (On Diff #523696) | Doesn't HasStdExtZvfh already imply HasStdExtZvfhmin? |
clang/include/clang/Basic/riscv_vector.td | ||
---|---|---|
2273–2279 | The semantics for RequiredFeatures is Features required to enable for this builtin. Since not all types in the range require the ZvfhminOrZvfh feature, it may make sense to do some refactoring: I think two possible solutions are:
|
clang/test/CodeGen/RISCV/rvv-intrinsics-handcrafted/zvfhmin-error.c | ||
---|---|---|
16 | Do we need a test that checks calls to __riscv_vfwcvt_f and __riscv_vfncvt_f using type vfloat16m1_t without zvfh nor zvfhmin lead to the expected error asking for zvfh or zvfhmin? |
I have left some comments on the clang side of this patch, but I think you are pretty close and have that under control.
I will take a closer look on what is left to do on the CodeGen side of things this week, and I will let you know if I have any comments and if I think there is any opportunity to collaborate here.
https://reviews.llvm.org/D151414 this is the backend part.
clang/include/clang/Basic/riscv_vector.td | ||
---|---|---|
2273–2279 | I split it into 2 definations. | |
clang/test/CodeGen/RISCV/rvv-intrinsics-handcrafted/zvfhmin-error.c | ||
16 | fp16 vector type check is in Sema/riscv-vector-float16-check.c. | |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
1018 ↗ | (On Diff #523696) | Fixed in https://reviews.llvm.org/D151414. |
2096 ↗ | (On Diff #523696) | Fixed in https://reviews.llvm.org/D151414. |
llvm/lib/Target/RISCV/RISCVSubtarget.h | ||
162 ↗ | (On Diff #523696) | The v spec doesn't metion this. |
llvm/lib/Target/RISCV/RISCVSubtarget.h | ||
---|---|---|
162 ↗ | (On Diff #523696) | I think the spec conveys this when it says The Zvfhmin extension depends on the Zve32f extension. |
llvm/lib/Target/RISCV/RISCVSubtarget.h | ||
---|---|---|
162 ↗ | (On Diff #523696) | My mistake, that says Zve32f, not Zvfh. However, the spec does say: When the Zvfhmin extension is implemented, the vfwcvt.f.f.v and vfncvt.f.f.w instructions become defined when SEW=16 and also says When the Zvfh extension is implemented, all instructions in Sections Vector Floating-Point Instructions. Since vfwcvt.f.f.v and vfncvt.f.f.w are part of Vector Floating-Point Instructions section, this is how it is implied. |
llvm/lib/Target/RISCV/RISCVSubtarget.h | ||
---|---|---|
162 ↗ | (On Diff #523696) | It's implemented in LLVM by this patch https://reviews.llvm.org/D150016 |
llvm/lib/Target/RISCV/RISCVSubtarget.h | ||
---|---|---|
162 ↗ | (On Diff #523696) | This patch make Zvfh imply Zfhmin not Zvfhmin. I think the relation between Zvfh and Zvfhmin is just like the relation between Zfh and Zfhmin. Now Zfh doesn't imply Zfhmin, so I keep Zvfh not imply Zvfhmin. |
llvm/lib/Target/RISCV/RISCVSubtarget.h | ||
---|---|---|
162 ↗ | (On Diff #523696) | Oops. Too many extensions with similar names. Thanks. |
clang/lib/Sema/SemaRISCVVectorLookup.cpp | ||
---|---|---|
207 | You can reuse HasZvfh here |
Sorry, I don't understand your comment well. I think that change you mentioned should not be included into this revision which only support zvfhmin for clang.
I think I was thinking of bfloat16. Sorry to many 16-bit float related patches lately.
Do we need to enable these intrinsics for Zvfhmin?
vfloat16mf4_t __riscv_vle16_v_f16mf4 (const float16_t *base, size_t vl); vfloat16mf2_t __riscv_vle16_v_f16mf2 (const float16_t *base, size_t vl); vfloat16m1_t __riscv_vle16_v_f16m1 (const float16_t *base, size_t vl); vfloat16m2_t __riscv_vle16_v_f16m2 (const float16_t *base, size_t vl); vfloat16m4_t __riscv_vle16_v_f16m4 (const float16_t *base, size_t vl); vfloat16m8_t __riscv_vle16_v_f16m8 (const float16_t *base, size_t vl);
Similar for all the other load/store intrinsics?
Also all the vreinterpret intrinsics.
And __riscv_vmerge_vvm_f16* and __riscv_vmv_v_v_f16*
They can be a separate patch.
Thank you for the patch. Few comments here.
clang/include/clang/Basic/riscv_vector.td | ||
---|---|---|
2274 | I think using ZvfhminOrZvfh is not accurate here. By the v-spec:
I think making it let RequiredFeatures = ["Zvfhmin"] would be clearer. | |
clang/lib/Sema/SemaRISCVVectorLookup.cpp | ||
206 | I think HasZvfhmin is a more accurate naming. | |
clang/test/CodeGen/RISCV/rvv-intrinsics-handcrafted/zvfhmin-error.c | ||
21 | If zvfhmin is not specified, should the compiler emit semantic error when encountering vfloat16*_t types? | |
clang/test/CodeGen/RISCV/rvv-intrinsics-handcrafted/zvfhmin.c | ||
16 | This test case is already covered. |
clang/lib/Sema/Sema.cpp | ||
---|---|---|
2050 ↗ | (On Diff #527312) | We can remove !TI.hasFeature("experimental-zvfh") since zvfh will imply zvfhmin. |
2050 ↗ | (On Diff #527312) | "zvfh or zvfhmin" -> "zvfhmin" |
clang/test/CodeGen/RISCV/rvv-intrinsics-handcrafted/zvfhmin-error.c | ||
21 | Sorry I missed the test case below. Please ignore this comment. |
clang/include/clang/Basic/riscv_vector.td | ||
---|---|---|
2274 | Note that the spec says Zfhmin(no v) not Zvfhmin. |
clang/include/clang/Basic/riscv_vector.td | ||
---|---|---|
2274 | My mistake. I suspect this is an oversight of the v-spec, just created an issue for this. |
clang/include/clang/Basic/riscv_vector.td | ||
---|---|---|
2274 | Thanks for comment. I think that v-spec is in accordance with the scalar spec to some degree. Since zfh doesn't imply zfhmin, I don't think they will let zvfh imply zvfhmin. | |
clang/test/CodeGen/RISCV/rvv-intrinsics-handcrafted/zvfhmin.c | ||
16 | Yes, but this test is for zvfhmin. vfncvt.c contains other convert cases not enable for zvfhmin so we can't just add a zvfhmin check. |
I think using ZvfhminOrZvfh is not accurate here. By the v-spec:
I think making it let RequiredFeatures = ["Zvfhmin"] would be clearer.