This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add Zvfhmin extension for clang
ClosedPublic

Authored by jacquesguan on May 10 2023, 12:39 AM.

Diff Detail

Event Timeline

jacquesguan created this revision.May 10 2023, 12:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2023, 12:39 AM
jacquesguan requested review of this revision.May 10 2023, 12:39 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 10 2023, 12:40 AM

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

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?

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?

Yes, in order to avoid duplicate work.

evandro removed a subscriber: evandro.May 17 2023, 3:56 PM

make f16 vector type legal for Zvfhmin, add predicates to enable 2 convert pattern.

@michaelmaitland , I update this revision with my local branch. May you have a look and see what missed compared with you version?

Fix indention.

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"?

Please split clang and llvm codegen into separate patches.

craig.topper added inline comments.May 23 2023, 1:31 PM
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:

  1. to split def of vfwcvt_f_f_v and vfncvt_f_f_w by type_range and the type range x uses the RequiredFeatures
  2. Use different required features for different type ranges (i.e. RequiredFeatures is a list of lists where the outer list is for each type in the range, and the inner list is the RequiredFeature for that type.)
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?

@michaelmaitland , I update this revision with my local branch. May you have a look and see what missed compared with you version?

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.

Split into 2 revisions and address comment.

jacquesguan retitled this revision from [RISCV] Add Zvfhmin extension. to [RISCV] Add Zvfhmin extension for clang..
jacquesguan edited the summary of this revision. (Show Details)
jacquesguan marked 8 inline comments as done.May 25 2023, 3:38 AM

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)
2096 ↗(On Diff #523696)
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.

craig.topper added inline comments.May 25 2023, 8:04 AM
llvm/lib/Target/RISCV/RISCVSubtarget.h
162 ↗(On Diff #523696)

It's implemented in LLVM by this patch https://reviews.llvm.org/D150016

jacquesguan marked 5 inline comments as done.May 25 2023, 7:06 PM
jacquesguan added inline comments.
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.

craig.topper added inline comments.May 25 2023, 7:37 PM
llvm/lib/Target/RISCV/RISCVSubtarget.h
162 ↗(On Diff #523696)

Oops. Too many extensions with similar names. Thanks.

This revision is now accepted and ready to land.May 30 2023, 9:41 AM
craig.topper requested changes to this revision.May 30 2023, 9:47 AM

We need to support reinterpret intrinsics so that we can load/store bf16 values.

This revision now requires changes to proceed.May 30 2023, 9:47 AM
craig.topper added inline comments.May 30 2023, 9:49 AM
clang/lib/Sema/SemaRISCVVectorLookup.cpp
207

You can reuse HasZvfh here

We need to support reinterpret intrinsics so that we can load/store bf16 values.

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.

We need to support reinterpret intrinsics so that we can load/store bf16 values.

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.

This revision is now accepted and ready to land.May 30 2023, 7:06 PM
craig.topper added a comment.EditedMay 30 2023, 7:08 PM

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.

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.

Yes, I will create a new patch to do that.

Address a comment.

This revision was landed with ongoing or failed builds.May 30 2023, 11:31 PM
This revision was automatically updated to reflect the committed changes.
craig.topper reopened this revision.May 31 2023, 10:18 AM

The backend patch must go before the clang patch.

This revision is now accepted and ready to land.May 31 2023, 10:18 AM

Update dependency.

eopXD added a comment.Jun 1 2023, 10:36 AM

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:

When the Zvfhmin extension is implemented, the vfwcvt.f.f.v and vfncvt.f.f.w instructions become defined when SEW=16.
The Zvfh extension depends on the Zve32f and Zfhmin extensions.

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
eopXD added a reviewer: eopXD.Jun 1 2023, 10:37 AM
eopXD added inline comments.Jun 1 2023, 10:41 AM
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.

craig.topper added inline comments.Jun 1 2023, 10:55 AM
clang/include/clang/Basic/riscv_vector.td
2274

Note that the spec says Zfhmin(no v) not Zvfhmin.

eopXD added inline comments.Jun 1 2023, 11:01 AM
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.

https://github.com/riscv/riscv-v-spec/issues/885

jacquesguan added inline comments.Jun 1 2023, 7:48 PM
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.

jacquesguan retitled this revision from [RISCV] Add Zvfhmin extension for clang. to [RISCV] Add Zvfhmin extension for clang.Aug 23 2023, 1:28 AM
This revision was landed with ongoing or failed builds.Aug 23 2023, 2:09 AM
This revision was automatically updated to reflect the committed changes.