Page MenuHomePhabricator

[RISCV] Add the zve extension according to the v1.0 spec
Needs ReviewPublic

Authored by eopXD on Oct 25 2021, 12:13 AM.

Details

Summary

zve is the new standard vector extension to specify varying degrees of vector support for embedding processors.
The zve extension is related to the zvl extension and other updates that are added in v1.0.

According to https://github.com/riscv-non-isa/riscv-c-api-doc/pull/21,
Clang defines macro __riscv_v_max_eew for zve and it can be used for applications that uses the vector extension.
Clang defines macro __riscv_v_max_eew_fp for zve and it can be used for applications that uses the vector extension.

Authored by: Zakk Chen <zakk.chen@sifive.com> @khchen
Co-Authored by: Eop Chen <eop.chen@sifive.com> @eopXD

Diff Detail

Unit TestsFailed

TimeTest
1,350 msx64 debian > AddressSanitizer-x86_64-linux-dynamic.TestCases::exitcode.cpp
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -shared-libasan -g -Wno-deprecated-declarations /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/asan/TestCases/exitcode.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxDynamicConfig/TestCases/Output/exitcode.cpp.tmp
1,760 msx64 debian > AddressSanitizer-x86_64-linux.TestCases::exitcode.cpp
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -g -Wno-deprecated-declarations /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/asan/TestCases/exitcode.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxConfig/TestCases/Output/exitcode.cpp.tmp

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
craig.topper added inline comments.Oct 25 2021, 1:57 PM
llvm/test/CodeGen/RISCV/rvv/fixed-vectors-bitcast.ll
158

I believe this is coming from the nan-boxing for f16 in RISCVTargetLowering::splitValueIntoRegisterParts. The addition of +f must have changed PartVT from i32/i64 to f32. Even though we're using i32 for the return due to ABI.

craig.topper added inline comments.Oct 25 2021, 3:23 PM
llvm/lib/Target/RISCV/RISCVSubtarget.h
141

I just put up D112496 to stop using hasStdExtV everywhere.

craig.topper added inline comments.Oct 25 2021, 3:25 PM
clang/lib/Basic/Targets/RISCV.cpp
184

Would't ELEN be the correct term here? Not EEW.

craig.topper added inline comments.Oct 25 2021, 3:28 PM
llvm/lib/Target/RISCV/RISCV.td
182

I don't think it is quite that simple. Couldn't you have a scalar D and have zve64f vector?

frasercrmck added inline comments.Oct 26 2021, 4:48 AM
llvm/lib/Target/RISCV/RISCV.td
182

Yes, that's fair. Though I still think we can create a more intuitive system of Predicates to handle the TableGen aspects, as you've begun to do in D112496.

llvm/lib/Target/RISCV/RISCVSubtarget.h
141

Ah right now I see what this was trying to do. I think your patch helps things, thanks.

llvm/test/CodeGen/RISCV/rvv/fixed-vectors-bitcast.ll
158

Ah, interesting. I can't tell if that's a bug fix, i.e., if it's invalid to compile this test without f - though shouldn't we pass experimental-zfh by that same logic? Regardless, maybe we could split this off and pre-commit it?

khchen added inline comments.Oct 26 2021, 9:41 AM
clang/lib/Basic/Targets/RISCV.cpp
181

please add a note in commit or comment here for those macros are proposed in the PR https://github.com/riscv-non-isa/riscv-c-api-doc/pull/21

184

https://github.com/riscv/riscv-v-spec/blob/master/v-spec.adoc#182-zve-vector-extensions-for-embedded-processors shows zve* extensions have Supported EEW, I guess it's why the term is EEW.

llvm/lib/Support/RISCVISAInfo.cpp
740–746

please update implication rule based on https://github.com/riscv/riscv-v-spec/issues/723#issuecomment-949542444

  1. The Zve32f and Zve64f extensions depend upon the F extension
  2. The Zve64d extension depends upon the D extension
  3. The V extension depends upon the F and D
craig.topper added inline comments.Oct 26 2021, 11:54 AM
clang/lib/Basic/Targets/RISCV.cpp
184

Is that because that section talks about them as a set of values rather than a single maximum?

eopXD added inline comments.Oct 27 2021, 12:54 AM
llvm/lib/Target/RISCV/RISCVSubtarget.h
141

Thank you @craig.topper for the patch.
LGTM, and we can have it landed after this series on zvl, zve patch are done.

eopXD edited the summary of this revision. (Show Details)Oct 27 2021, 1:10 AM
eopXD updated this revision to Diff 382545.Oct 27 2021, 1:13 AM

Rebase.

eopXD updated this revision to Diff 382565.Oct 27 2021, 2:04 AM
eopXD marked 2 inline comments as done.

Rebase.

Address comment:

  • implication rules for v and zve
    • zve64d imply d
    • zve*f imply f
    • d imply f
    • so since v imply zve64d, meaning v imply f and d
llvm/lib/Support/RISCVISAInfo.cpp
740–746

Thanks for the reminder. Addressed.

llvm/test/CodeGen/RISCV/attributes.ll
8

You are right. This should not be changed.

Removed.

eopXD updated this revision to Diff 382566.Oct 27 2021, 2:16 AM

Remove dependency checks because new implications added.

I think the rest of my comments would be to do with zvl so I'll leave it there to avoid repetition.

llvm/include/llvm/Support/RISCVISAInfo.h
65

Aside from the discussion about EEW vs. ELEN, something about the capitalization irks me. I realise we already have XLen but Eew looks... wrong. If other people disagree then that's fine.

llvm/lib/Support/RISCVISAInfo.cpp
67

Should this be in this patch? Or has some rebasing gone wrong and introduced code for D108694?

461

I'm not the most familiar with this API, but do we really need to checkDependency here when it's done in the next line?

686

Same here. Duplicate checkDependency?

750

Again, should zvl code be in this patch?

780

Really minor, but here you're using auto for StringRef but earlier and elsewhere it's auto &. I'm not sure which is preferred. Presumably StringRefs are cheap to copy and auto is fine? If auto & is more prominent in this file then go with that.

802

zvl patch?

eopXD added a comment.Oct 27 2021, 2:27 AM

Sorry I mixed zvl patch when rebasing. I will remove it ASAP.

eopXD updated this revision to Diff 382580.Oct 27 2021, 2:53 AM

Rebase correctly.

eopXD updated this revision to Diff 382583.Oct 27 2021, 2:57 AM
eopXD marked 3 inline comments as done.

Rebase.

eopXD marked 3 inline comments as done.Oct 27 2021, 2:59 AM
eopXD marked an inline comment as done.Oct 27 2021, 3:04 AM
eopXD added inline comments.
clang/lib/Basic/Targets/RISCV.cpp
181

Added in commit message.

llvm/include/llvm/Support/RISCVISAInfo.h
65

Hi Fraser,
FYI, I think there is a discussion happening here.

eopXD updated this revision to Diff 382620.Oct 27 2021, 5:29 AM

Under RISCV.td:
FeatureExtZve* -> FeatureStdExtZve*

eopXD updated this revision to Diff 382621.Oct 27 2021, 5:30 AM

Clang-format.

eopXD updated this revision to Diff 382631.Oct 27 2021, 5:55 AM

Rebase.

eopXD updated this revision to Diff 382636.Oct 27 2021, 5:58 AM

Rebase.

craig.topper added inline comments.Oct 27 2021, 10:57 AM
llvm/lib/Target/RISCV/RISCVSubtarget.h
141

I'd like to land my patch first and make this patch change hasVInstructions*() in the appropriate ways. If we don't do my patch first then this patch will enable i64 vectors with Zve32x which would be incorrect.

eopXD updated this revision to Diff 383596.Sat, Oct 30, 9:38 AM

Changes:

  • Modify zve related predicate based on better interfaces by D112496.
eopXD updated this revision to Diff 383955.Mon, Nov 1, 8:46 PM

Changes:

  • Rebase
  • Since now the minimal extension to include is zve32x, zvlsseg and zvamo no longer implies v in RISCV.td
  • Add implication: zve32x implies zvlsseg
eopXD updated this revision to Diff 383956.Mon, Nov 1, 9:00 PM

Update testcase since now zvlsseg don't imply v,
the testcases need to explicitly specify v.

eopXD updated this revision to Diff 383968.Mon, Nov 1, 10:20 PM

Fix test case fail:

  • delete test case in`riscv-target-features.c` that specifies zvlsseg alone
  • remove implication of zvlsseg -> v and zvamo -> v in RISCVISAInfo::toFeatures
eopXD retitled this revision from [WIP][RISCV] Add the zve extension according to the v1.0-rc2 spec to [RISCV] Add the zve extension according to the v1.0-rc2 spec.Tue, Nov 2, 12:09 AM
eopXD edited the summary of this revision. (Show Details)
eopXD retitled this revision from [RISCV] Add the zve extension according to the v1.0-rc2 spec to [RISCV] Add the zve extension according to the v1.0 spec.Tue, Nov 2, 1:29 AM
eopXD edited the summary of this revision. (Show Details)
craig.topper added inline comments.Wed, Nov 3, 12:02 PM
llvm/lib/Target/RISCV/RISCVSubtarget.h
170

This needs to be the same as hasVInstructionsF32.

craig.topper added inline comments.Wed, Nov 3, 12:20 PM
llvm/lib/Target/RISCV/RISCV.td
224

StdExtV depends on Zve64d which depends on FeatureStdExtZve64f which depends on FeatureStdExtZve32f which depends on FeatureStdExtZve32x. Do we need to check both hasStdExtV and hasStdExtZve32x here or could we just check hasStdExtZve32x?

craig.topper added inline comments.Wed, Nov 3, 12:27 PM
llvm/lib/Target/RISCV/RISCV.td
223

Can we add the AssemblerPredicate to HasVInstructions and use that? Or we can rename it to HasVInstructionsAnyInt? Similar for HasStdExtZveFloating and HasVInstructionsAnyF.

And maybe rename HasStdExtVIntegerEEW64 to something like HasVInstructionsEEW64?

I'd like to avoid using the term "StdExt" in the name for something that isn't truly an extension name.

eopXD updated this revision to Diff 384665.Wed, Nov 3, 10:35 PM

Address @craig.topper 's comments on predicates in RISCV.td

eopXD marked 3 inline comments as done.Wed, Nov 3, 10:46 PM
eopXD updated this revision to Diff 384972.Thu, Nov 4, 10:38 PM

Change:
After adding extension zve, the minimum requirement of vector instructions isn't v.
Let __riscv_vector be indicator if any vector extension is available.

Sent a proposal to the riscv-c-api-doc: https://github.com/riscv-non-isa/riscv-c-api-doc/pull/22

eopXD updated this revision to Diff 385255.Sat, Nov 6, 2:55 AM

Update test case.

eopXD updated this revision to Diff 385257.Sat, Nov 6, 3:08 AM

Update code - add dependency checks in RISCVISAInfo

eopXD updated this revision to Diff 385274.Sat, Nov 6, 7:34 AM

Update testcase.