Details

Reviewers

kito-cheng
HsiangKai
craig.topper

Commits

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

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

Repository: rG LLVM Github Monorepo

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Update testcase.

Harbormaster completed remote builds in B132841: Diff 385274.Nov 6 2021, 8:19 AM

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

Harbormaster completed remote builds in B143588: Diff 400279.Jan 15 2022, 7:21 AM

Rebase to latest main.

Harbormaster completed remote builds in B143590: Diff 400281.Jan 15 2022, 7:22 AM

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.

Rebase.

Harbormaster completed remote builds in B143591: Diff 400282.Jan 15 2022, 7:36 AM

eopXD removed a parent revision: D108694: [RISCV] Add the zvl extension according to the v1.0 spec.Jan 15 2022, 7:37 AM

Rebase again due to patch application fail.

Harbormaster completed remote builds in B143592: Diff 400283.Jan 15 2022, 8:27 AM

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

Harbormaster completed remote builds in B143607: Diff 400302.Jan 15 2022, 10:32 AM

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?

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

Herald added a subscriber: alextsao1999. · View Herald TranscriptJan 16 2022, 2:14 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)

Update code.

Harbormaster completed remote builds in B143658: Diff 400361.Jan 16 2022, 3:12 AM

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.
859	Why is there no Predicate on these aliases?
1499–1516	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]>`.
881	EEW64 indexed loads/stores also require RV64

Address comments.
Thanks for reviewing!

Harbormaster completed remote builds in B143708: Diff 400418.Jan 16 2022, 7:55 PM

eopXD edited the summary of this revision. (Show Details)Jan 17 2022, 9:43 AM

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 ;)

Harbormaster completed remote builds in B143832: Diff 400595.Jan 17 2022, 10:22 AM

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

Rebase.

Harbormaster completed remote builds in B144079: Diff 400937.Jan 18 2022, 12:25 PM

craig.topper added inline comments.Jan 18 2022, 9:54 PM

llvm/lib/Target/RISCV/RISCVInstrInfoV.td
1518	The unit-stride and strided with EEW=64 don't require RV64. Just the indexed versions.
llvm/lib/Target/RISCV/RISCVSubtarget.h
193	This needs to check Zve32f not Zve32x I think?
194	This should check Zve64d not Zve64x I think?
198	Leave this line as an alias to hasVInstructionsF32()

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

llvm/lib/Target/RISCV/RISCVInstrInfoV.td
1518	Thank you for catching this.

Harbormaster completed remote builds in B144206: Diff 401104.Jan 18 2022, 11:38 PM

Fix testcase failure and bug.

Harbormaster completed remote builds in B144324: Diff 401271.Jan 19 2022, 11:24 AM

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
864	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
192	Zve32f I think since Zfh requires F.
193	I think we need to check HasStdExtF with Zve32f with a FIXME to consider HasStdExtZfinx in the future.
194	I think we need to check HasStdExtD with Zve64d with a FIXME to consider HasStdExtZdinx in the future

Address @craig.topper 's comments.

craig.topper added inline comments.Jan 19 2022, 10:27 PM

llvm/lib/Support/RISCVISAInfo.cpp
744	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
195	Zfinx -> Zdinx

achieveartificialintelligence added inline comments.Jan 19 2022, 10:33 PM

llvm/test/CodeGen/RISCV/rvv/vloxseg-rv32.ll
2	Do we need `+f` here?

Harbormaster completed remote builds in B144484: Diff 401507.Jan 19 2022, 10:40 PM

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.

LGTM

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.

Rebase to latest main.

Harbormaster completed remote builds in B144495: Diff 401519.Jan 19 2022, 11:47 PM

Closed by commit rG8eae99dfe541: [RISCV] Add the zve extension according to the v1.0 spec (authored by eopXD). · Explain WhyJan 19 2022, 11:48 PM

This revision was automatically updated to reflect the committed changes.

eopXD added a commit: rG8eae99dfe541: [RISCV] Add the zve extension according to the v1.0 spec.

eopXD removed a child revision: D113336: [RISCV] Imply extensions in RISCVTargetInfo::initFeatureMap.Jan 19 2022, 11:57 PM

eopXD edited the summary of this revision. (Show Details)Jan 20 2022, 10:25 AM

eopXD mentioned this in rG8eae99dfe541: [RISCV] Add the zve extension according to the v1.0 spec.Jan 20 2022, 10:45 AM

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

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 401524

clang/lib/Basic/Targets/RISCV.cpp

clang/test/Driver/riscv-arch.c

clang/test/Preprocessor/riscv-target-features.c

llvm/include/llvm/Support/RISCVISAInfo.h

llvm/lib/Support/RISCVISAInfo.cpp

llvm/lib/Target/RISCV/RISCV.td

llvm/lib/Target/RISCV/RISCVInstrInfoV.td

llvm/lib/Target/RISCV/RISCVSchedRocket.td

llvm/lib/Target/RISCV/RISCVSubtarget.h

llvm/test/CodeGen/RISCV/attributes.ll

llvm/test/CodeGen/RISCV/rvv/ctlz-sdnode.ll

llvm/test/CodeGen/RISCV/rvv/cttz-sdnode.ll

llvm/test/CodeGen/RISCV/rvv/fixed-vectors-ctlz.ll

llvm/test/CodeGen/RISCV/rvv/fixed-vectors-cttz.ll

llvm/test/CodeGen/RISCV/rvv/regalloc-fast-crash.ll

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

llvm/test/CodeGen/RISCV/rvv/vloxseg-rv64.ll

llvm/test/CodeGen/RISCV/rvv/vlseg-rv32.ll

llvm/test/CodeGen/RISCV/rvv/vlseg-rv64.ll

llvm/test/CodeGen/RISCV/rvv/vlsegff-rv32-dead.ll

llvm/test/CodeGen/RISCV/rvv/vlsegff-rv32.ll

llvm/test/CodeGen/RISCV/rvv/vlsegff-rv64-dead.ll

llvm/test/CodeGen/RISCV/rvv/vlsegff-rv64.ll

llvm/test/CodeGen/RISCV/rvv/vlsseg-rv32.ll

llvm/test/CodeGen/RISCV/rvv/vlsseg-rv64.ll

llvm/test/CodeGen/RISCV/rvv/vluxseg-rv32.ll

llvm/test/CodeGen/RISCV/rvv/vluxseg-rv64.ll

llvm/test/CodeGen/RISCV/rvv/vsoxseg-rv32.ll

llvm/test/CodeGen/RISCV/rvv/vsoxseg-rv64.ll

llvm/test/CodeGen/RISCV/rvv/vsseg-rv32.ll

llvm/test/CodeGen/RISCV/rvv/vsseg-rv64.ll

llvm/test/CodeGen/RISCV/rvv/vssseg-rv32.ll

llvm/test/CodeGen/RISCV/rvv/vssseg-rv64.ll

llvm/test/CodeGen/RISCV/rvv/vsuxseg-rv32.ll

llvm/test/CodeGen/RISCV/rvv/vsuxseg-rv64.ll

llvm/test/CodeGen/RISCV/rvv/zvlsseg-zero-vl.ll

llvm/test/MC/RISCV/attribute-arch.s

llvm/test/MC/RISCV/rvv/add.s

llvm/test/MC/RISCV/rvv/and.s

llvm/test/MC/RISCV/rvv/clip.s

llvm/test/MC/RISCV/rvv/compare.s

llvm/test/MC/RISCV/rvv/convert.s

llvm/test/MC/RISCV/rvv/div.s

llvm/test/MC/RISCV/rvv/ext.s

llvm/test/MC/RISCV/rvv/fadd.s

llvm/test/MC/RISCV/rvv/fcompare.s

llvm/test/MC/RISCV/rvv/fdiv.s

llvm/test/MC/RISCV/rvv/fmacc.s

llvm/test/MC/RISCV/rvv/fminmax.s

llvm/test/MC/RISCV/rvv/fmul.s

llvm/test/MC/RISCV/rvv/fmv.s

llvm/test/MC/RISCV/rvv/fothers.s

llvm/test/MC/RISCV/rvv/freduction.s

llvm/test/MC/RISCV/rvv/fsub.s

llvm/test/MC/RISCV/rvv/invalid-eew.s

llvm/test/MC/RISCV/rvv/load.s

llvm/test/MC/RISCV/rvv/macc.s

llvm/test/MC/RISCV/rvv/mask.s

llvm/test/MC/RISCV/rvv/minmax.s

llvm/test/MC/RISCV/rvv/mul.s

llvm/test/MC/RISCV/rvv/mv.s

llvm/test/MC/RISCV/rvv/or.s

llvm/test/MC/RISCV/rvv/others.s

llvm/test/MC/RISCV/rvv/reduction.s

llvm/test/MC/RISCV/rvv/shift.s

llvm/test/MC/RISCV/rvv/sign-injection.s

llvm/test/MC/RISCV/rvv/store.s

llvm/test/MC/RISCV/rvv/sub.s

llvm/test/MC/RISCV/rvv/vsetvl.s

llvm/test/MC/RISCV/rvv/xor.s

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

Revision Contents
Changeset List