Page MenuHomePhabricator

[RISCV] Decouple Zve* extensions and the V extension.
ClosedPublic

Authored by jacquesguan on Jan 20 2022, 10:42 PM.

Details

Summary

According to the spec, there are some difference between V and Zve64d. For example, the vmulh integer multiply variants that return the high word of the product (vmulh.vv, vmulh.vx, vmulhu.vv, vmulhu.vx, vmulhsu.vv, vmulhsu.vx) are not included for EEW=64 in Zve64*, but V extension does support these instructions. So I think that we could not mixed use Zve64d and the V extension.

Diff Detail

Event Timeline

jacquesguan created this revision.Jan 20 2022, 10:42 PM
jacquesguan requested review of this revision.Jan 20 2022, 10:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2022, 10:42 PM

FeatureStdExtV will imply Zve32x, Zve64x, Zve32f and Zve64d, both RISCV.td or RISCVISAInfo does so.
So I wonder why do we need the extra condition here.
May you tell more?

The feature implication here is only used for llc .ll tests. Clang still thinks they are connected via the code in RISCVISAInfo.cpp

FeatureStdExtV will imply Zve32x, Zve64x, Zve32f and Zve64d, both RISCV.td or RISCVISAInfo does so.
So I wonder why do we need the extra condition here.
May you tell more?

For example, if I want to make vmulhsu.vv disable for Zve64d when EEW=64, I need some predicates or condition asserts to make sure that current target just implement only Zve64d but not the V extension.

I see your point now.

In the v-spec describing for Zve:

All Zve* extensions support all vector integer instructions (Section Vector Integer Arithmetic Instructions), except that the vmulh integer multiply variants that return the high word of the product (vmulh.vv, vmulh.vx, vmulhu.vv, vmulhu.vx, vmulhsu.vv, vmulhsu.vx) are not included for EEW=64 in Zve64*.

For V extension:

The V extension supports all vector integer instructions (Section Vector Integer Arithmetic Instructions).

So we need to have something to express that V is enabled.


If I have parsed correctly in the above, I think we should have something that indicates V ext. is specified rather than adding HasStdExtV to the existing predicates.

I see your point now.

In the v-spec describing for Zve:

All Zve* extensions support all vector integer instructions (Section Vector Integer Arithmetic Instructions), except that the vmulh integer multiply variants that return the high word of the product (vmulh.vv, vmulh.vx, vmulhu.vv, vmulhu.vx, vmulhsu.vv, vmulhsu.vx) are not included for EEW=64 in Zve64*.

For V extension:

The V extension supports all vector integer instructions (Section Vector Integer Arithmetic Instructions).

So we need to have something to express that V is enabled.


If I have parsed correctly in the above, I think we should have something that indicates V ext. is specified rather than adding HasStdExtV to the existing predicates.

In my opinion, FeatureStdExtV just means that we have the V extension. In the instruction level, the V extension includes all the Zve* extensions. But they still have different using space which V is for application processors and Zve* is for embedded processors. Maybe it's better to not mix these two things?

In my opinion, FeatureStdExtV just means that we have the V extension. In the instruction level, the V extension includes all the Zve* extensions. But they still have different using space which V is for application processors and Zve* is for embedded processors. Maybe it's better to not mix these two things?

Yes I agree with you that they are different and shouldn't be mixed.
On the other hand, if we want to guard vmulh out of Zve* extension shouldn't we use FeatureStdExtV?

In my opinion, FeatureStdExtV just means that we have the V extension. In the instruction level, the V extension includes all the Zve* extensions. But they still have different using space which V is for application processors and Zve* is for embedded processors. Maybe it's better to not mix these two things?

Yes I agree with you that they are different and shouldn't be mixed.
On the other hand, if we want to guard vmulh out of Zve* extension shouldn't we use FeatureStdExtV?

Yes, this is why I remove FeatureStdExtZve64d from the implies of FeatureStdExtV, for normal v instructions, we have HasVInstructions* to handle it, and for the instructions that only enable for V extension, we could use FeatureStdExtV.

Without removing the implication Zve64d we can still guard things using FeatureStdExtV right?
Zve64d does not imply V.

Thanks for pointing out that Zve maybe shouldn’t be a “subset” of V.
The patch makes sense to me now.
I guess you will have to modify the implications under RISCVISAInfo.cpp too?

eopXD added a comment.EditedJan 21 2022, 1:16 AM

I guess you will have to modify the implications under RISCVISAInfo.cpp too?

I was wrong. I think we don't need to modify any implications there.

eopXD accepted this revision.EditedJan 21 2022, 1:35 AM

LGTM, I have created D117865 to fix the mis-guarded instructions .
Thank you for the patch and explaination!

This revision is now accepted and ready to land.Jan 21 2022, 1:35 AM
craig.topper requested changes to this revision.Jan 21 2022, 8:31 AM

If we don’t change the implications in RISCVISAInfo then clang will still always enable Zve when V is in the march string. Is that what we want?

This revision now requires changes to proceed.Jan 21 2022, 8:31 AM
eopXD added a comment.Jan 21 2022, 9:09 AM

A question that bothers me is that we need to specify a specific feature to enable the TARGET_BUILTIN for rvv, if we are removing the “subset” relationship here, how can V enable the builtin-s?

A question that bothers me is that we need to specify a specific feature to enable the TARGET_BUILTIN for rvv, if we are removing the “subset” relationship here, how can V enable the builtin-s?

If I remember right "experimental-zve32x|experimental-v" should be valid syntax. Though I'm not sure the string is used at all because of how the clang_builtin_alias used by the header works.

eopXD added a comment.Jan 21 2022, 9:24 AM

A question that bothers me is that we need to specify a specific feature to enable the TARGET_BUILTIN for rvv, if we are removing the “subset” relationship here, how can V enable the builtin-s?

If I remember right "experimental-zve32x|experimental-v" should be valid syntax. Though I'm not sure the string is used at all because of how the clang_builtin_alias used by the header works.

I see. I will change my patch and see what I should do to address your comments.
Thank you.

Address comment.

Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2022, 8:03 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

A question that bothers me is that we need to specify a specific feature to enable the TARGET_BUILTIN for rvv, if we are removing the “subset” relationship here, how can V enable the builtin-s?

If I remember right "experimental-zve32x|experimental-v" should be valid syntax. Though I'm not sure the string is used at all because of how the clang_builtin_alias used by the header works.

I modified the RISCVISAInfo and make this revision works for Clang.

Fix test cases.

Maybe change the title to address more specifically of what this patch does?

jacquesguan retitled this revision from [RISCV] Refactor Zve* extensions. to [RISCV] Decouple Zve* extensions and the V extension..Jan 21 2022, 10:38 PM

Maybe change the title to address more specifically of what this patch does?

Done, thanks.

craig.topper added inline comments.Jan 21 2022, 10:41 PM
clang/lib/Sema/SemaChecking.cpp
3979–3987

Is this change testable?

llvm/lib/Support/RISCVISAInfo.cpp
710

Should we check that Zve and V are not specified together?

Address comment

jacquesguan added inline comments.Jan 22 2022, 12:02 AM
clang/lib/Sema/SemaChecking.cpp
3979–3987

Done, I add a test case clang/test/CodeGen/RISCV/rvv-intrinsics/rvv-error.c to test this.

llvm/lib/Support/RISCVISAInfo.cpp
710

Done.

Fix test case

craig.topper added inline comments.Jan 23 2022, 6:51 PM
llvm/lib/Target/RISCV/RISCVSubtarget.h
205

This should be (HasStdExtV || HasStdExtZve32f) && HasStdExtZfh. V does mean f16 vectors are enabled.

Address comment

This revision is now accepted and ready to land.Jan 23 2022, 7:34 PM
This revision was landed with ongoing or failed builds.Jan 23 2022, 11:07 PM
This revision was automatically updated to reflect the committed changes.