This is an archive of the discontinued LLVM Phabricator instance.

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

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_elen, __riscv_v_elen_fp for zve and
it can be used by 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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
frasercrmck added inline comments.Oct 25 2021, 2:01 AM
llvm/lib/Target/RISCV/RISCV.td
164

Do we need to define distinct SubtargetFeatures for each of these extensions or could they be broken down into a single MaxEEW feature (32 or 64) in conjunction with the pre-existing F/D features. This seems like it's more complicated than it needs to be.

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

Is this correct? I thought we'd keep hasStdExtV as being the single-letter V extension, and Zve32x isn't that.

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

Why is this being changed in this patch?

craig.topper added inline comments.Oct 25 2021, 1:57 PM
llvm/test/CodeGen/RISCV/rvv/fixed-vectors-bitcast.ll
158 ↗(On Diff #381871)

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
158

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
164

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
164

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
158

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 ↗(On Diff #381871)

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
729–750

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
158

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
729–750

Thanks for the reminder. Addressed.

llvm/test/CodeGen/RISCV/attributes.ll
7–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
70

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

458

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?

683

Same here. Duplicate checkDependency?

792

Again, should zvl code be in this patch?

801

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.

825

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
158

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.Oct 30 2021, 9:38 AM

Changes:

  • Modify zve related predicate based on better interfaces by D112496.
eopXD updated this revision to Diff 383955.Nov 1 2021, 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.Nov 1 2021, 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.Nov 1 2021, 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.Nov 2 2021, 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.Nov 2 2021, 1:29 AM
eopXD edited the summary of this revision. (Show Details)
craig.topper added inline comments.Nov 3 2021, 12:02 PM
llvm/lib/Target/RISCV/RISCVSubtarget.h
186–187

This needs to be the same as hasVInstructionsF32.

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

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.Nov 3 2021, 12:27 PM
llvm/lib/Target/RISCV/RISCV.td
167

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.Nov 3 2021, 10:35 PM

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

eopXD marked 3 inline comments as done.Nov 3 2021, 10:46 PM
eopXD updated this revision to Diff 384972.Nov 4 2021, 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.Nov 6 2021, 2:55 AM

Update test case.

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

Update code - add dependency checks in RISCVISAInfo

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

Update testcase.

eopXD updated this revision to Diff 400279.Jan 15 2022, 7:20 AM

Rebase to latest main.
Resolve conflict due to deletion of zvamo and addition of zfh, zfhmin.

eopXD updated this revision to Diff 400281.Jan 15 2022, 7:21 AM

Rebase to latest main.

eopXD marked 7 inline comments as done.Jan 15 2022, 7:33 AM
eopXD added inline comments.
clang/lib/Basic/Targets/RISCV.cpp
184

I think Zve is restricting the EEW, not ELEN.

eopXD updated this revision to Diff 400282.Jan 15 2022, 7:35 AM

Rebase.

eopXD updated this revision to Diff 400283.Jan 15 2022, 7:37 AM

Rebase again due to patch application fail.

eopXD updated this revision to Diff 400302.Jan 15 2022, 9:33 AM

Update test case since v extension now depends on (implies) f and d extension.

craig.topper added inline comments.Jan 15 2022, 11:00 AM
clang/lib/Basic/Targets/RISCV.cpp
184

The spec defines ELEN as "The maximum size in bits of a vector element that any operation can produce or consume" That sounds like maximum EEW to me.

This statement appears in section 3.4.2

"For standard vector extensions with
ELEN=32, fractional LMULs of 1/2 and 1/4 must be supported. For standard vector extensions with ELEN=64, fractional
LMULs of 1/2, 1/4, and 1/8 must be supported."

I take "standard vector extensions with ELEN=32" to mean Zve32x, Zve32f.

And "standard vector extensions with ELEN=64" to mean Zve64x, Zve64f, Zve64d, and V.

Am I interpreting that incorrectly?

eopXD marked an inline comment as not done.Jan 15 2022, 11:07 AM
eopXD added inline comments.
clang/lib/Basic/Targets/RISCV.cpp
184

Yes I think you are correct. Maximum EEW is an alias of ELEN.

I see that the discussion in https://github.com/riscv-non-isa/riscv-c-api-doc/pull/21 hasn't conclude on whether changing the name into elen and elen_fp.

Should this patch pend until conclusions are drawn?

eopXD updated this revision to Diff 400360.Jan 16 2022, 2:14 AM

Remove macro related code, lets add them later when conclusions are drawn.

eopXD added a comment.Jan 16 2022, 2:16 AM

We can land non-macro related code for zve first and continue on proceeding patches.

eopXD retitled this revision from [RISCV] Add the zve extension according to the v1.0 spec to [RISCV][MC] Add the zve extension according to the v1.0 spec.Jan 16 2022, 2:20 AM
eopXD edited the summary of this revision. (Show Details)
eopXD updated this revision to Diff 400361.Jan 16 2022, 2:21 AM

Update code.

craig.topper added inline comments.Jan 16 2022, 11:40 AM
llvm/include/llvm/Support/RISCVISAInfo.h
95

There's no definition for this

llvm/lib/Target/RISCV/RISCVInstrInfoV.td
821

Is this deletion correct? I don't see where the VLUXEI64 instructions are declared now.

871

Why is there no Predicate on these aliases?

1509

These require RV64 don't they?

craig.topper added inline comments.Jan 16 2022, 11:43 AM
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
353

Why is this class only used for [64]

821

Oh I see, it's done with VIndexLoadStore<[64]>.

893

EEW64 indexed loads/stores also require RV64

eopXD updated this revision to Diff 400418.Jan 16 2022, 7:04 PM
eopXD marked 7 inline comments as done.

Address comments.
Thanks for reviewing!

eopXD edited the summary of this revision. (Show Details)Jan 17 2022, 9:43 AM
eopXD updated this revision to Diff 400595.Jan 17 2022, 9:45 AM
eopXD marked 5 inline comments as done.

Update code

  • Add macro __riscv_v_elen and __riscv_v_elen_fp
eopXD added inline comments.Jan 17 2022, 9:56 AM
clang/lib/Basic/Targets/RISCV.cpp
184

I see that the discussion has now concluded.
Thank you @craig.topper for pinging at the c-api PR.
Changing macro to __riscv_v_elen and __riscv_v_elen_fp

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

Since we are changing to ELEN now I hope its less disturbing to you and I can resolve this ;)

eopXD retitled this revision from [RISCV][MC] Add the zve extension according to the v1.0 spec to [RISCV] Add the zve extension according to the v1.0 spec.Jan 18 2022, 11:52 AM
craig.topper added inline comments.Jan 18 2022, 9:54 PM
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
1511

The unit-stride and strided with EEW=64 don't require RV64. Just the indexed versions.

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

This needs to check Zve32f not Zve32x I think?

185

This should check Zve64d not Zve64x I think?

187

Leave this line as an alias to hasVInstructionsF32()

eopXD updated this revision to Diff 401104.Jan 18 2022, 11:05 PM
eopXD marked 4 inline comments as done.

Address @craig.topper 's comments.
Thank you for reviewing.

llvm/lib/Target/RISCV/RISCVInstrInfoV.td
1511

Thank you for catching this.

eopXD updated this revision to Diff 401271.Jan 19 2022, 8:58 AM

Fix testcase failure and bug.

craig.topper added inline comments.Jan 19 2022, 11:49 AM
llvm/lib/Target/RISCV/RISCV.td
186

Zve32f requires F or Zfinx. It can't imply F.

206

Zve64d requires D or Zdinx. It can't imply D.

209

V can imply F and D though I think.

llvm/lib/Target/RISCV/RISCVInstrInfoV.td
901

Only the indexed load/stores require RV64 for EEW=64. Sorry I missed that when I caught it for the segment load/store.

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

Zve32f I think since Zfh requires F.

184

I think we need to check HasStdExtF with Zve32f with a FIXME to consider HasStdExtZfinx in the future.

185

I think we need to check HasStdExtD with Zve64d with a FIXME to consider HasStdExtZdinx in the future

eopXD updated this revision to Diff 401507.Jan 19 2022, 9:51 PM
eopXD marked 7 inline comments as done.

Address @craig.topper 's comments.

craig.topper added inline comments.Jan 19 2022, 10:27 PM
llvm/lib/Support/RISCVISAInfo.cpp
731

I think we need to drop the implication of "f" here and "d" on zve64f. Move them to V

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

Zfinx -> Zdinx

llvm/test/CodeGen/RISCV/rvv/vloxseg-rv32.ll
2

Do we need +f here?

eopXD updated this revision to Diff 401514.Jan 19 2022, 10:42 PM
eopXD marked 2 inline comments as done.

Address more of Craig's comments.

eopXD added inline comments.Jan 19 2022, 10:48 PM
llvm/test/CodeGen/RISCV/rvv/vloxseg-rv32.ll
2

Yes. D should imply F now. Let me create a another patch for it as the current patch is for zve.

This revision is now accepted and ready to land.Jan 19 2022, 10:53 PM
craig.topper added inline comments.Jan 19 2022, 10:54 PM
llvm/test/CodeGen/RISCV/rvv/vloxseg-rv32.ll
2

D already implies F in RISCV.td which is what is used for -mattr.

eopXD updated this revision to Diff 401519.Jan 19 2022, 11:04 PM
eopXD marked an inline comment as not done.

Rebase to latest main.

This revision was automatically updated to reflect the committed changes.
eopXD edited the summary of this revision. (Show Details)Jan 20 2022, 10:25 AM