Page MenuHomePhabricator

[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
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.

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

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

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
1518

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

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

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