Page MenuHomePhabricator

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

Authored by eopXD on Aug 25 2021, 4:36 AM.

Details

Summary

zvl is the new standard vector extension that specifies the minimum vector length of the vector extension.
The zvl extension is related to the zve 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_min_vlen for zvl and it can be used for applications that uses the vector extension.
LLVM checks whether the option riscv-v-vector-bits-min (if specified) matches the zvl* extension specified.

Diff Detail

Unit TestsFailed

TimeTest
1,770 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,980 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
eopXD updated this revision to Diff 368608.Aug 25 2021, 4:45 AM

Update code, bug fixes to this patch.

eopXD added inline comments.Aug 25 2021, 4:49 AM
llvm/lib/Target/RISCV/RISCV.td
214

Don't need the AssemblerPredicate here because this sub-extension is used to restrict vlen, which is treated as a constant in the assembly.

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

I don't think this function will be called. However other sub-extensions will have this function so I still created it.

kito-cheng added inline comments.Aug 25 2021, 5:36 AM
llvm/lib/Support/RISCVISAInfo.cpp
72

Although the table in vector spec only list zvl32b~zvl1024b, but there is note say Longer vector length extensions should follow the same pattern.[1], so I would suggest let enumerate to 65536, the unbound of VLEN[2],

[1] https://github.com/riscv/riscv-v-spec/blob/master/v-spec.adoc#181-zvl-minimum-vector-length-standard-extensions
[2] https://github.com/riscv/riscv-v-spec/blob/master/v-spec.adoc#2-implementation-defined-constant-parameters

llvm/lib/Target/RISCV/RISCVSubtarget.cpp
143

I guess this should be more than an assertion? but I am not sure does it make sense to emit error or warning here? or just silently return ZvlLen if RVVVectorBitsMin is less than ZvlLen.

kito-cheng added inline comments.Aug 25 2021, 7:02 PM
llvm/lib/Support/RISCVISAInfo.cpp
72

Oh, typo, unbound should be upper bound :p

eopXD updated this revision to Diff 368796.Aug 25 2021, 8:03 PM
eopXD marked 2 inline comments as done.

Address comments.

  • enumerate zvl up to 65536
  • fix grammitcal error in assertion error message
llvm/lib/Target/RISCV/RISCVSubtarget.cpp
143

I think the compiler should not return ZvlLen silently if the specified RVVVectorBitsMin is less. As zvl hard restricts the minimum vlen, the user should be aware (notified) of that. I think the assertion error here can guide the user to either increase their RVVVectorBitsMin in the argument or compile under another architecture specification.

craig.topper added inline comments.
llvm/lib/Target/RISCV/RISCVSubtarget.cpp
118–121

The max should be greater than ZvlLen right?

136

Some of the callers of this will break if it returns a value less than 64. We need to limit ELEN to 32 if VLEN is 32. And we can't use LMUL=1/8 with i8 vectors, or LMUL=1/4 with i16 vector, or LMUL=1/2 with i32 vector.

143

assertions are compiled out of release builds. The code after the assertions was trying to do something sane for release builds.

I think we need to visit some larger aspects of our vector implementation. Here are some thoughts.

-Most uses of Subtarget.hasStdExtV() don't really mean what the spec calls the standard V extension. They just means that we have vector instructions. Could be V, could be one of the Zve32* or Zve64* extensions.
-V extension passed to -march should imply at least Zvl128b.
-V extension passed to -march should enable F and D.
-Does Zvl32b passed to march enable vector instructions? Or do we still need Zve32* or Zve64* or V?
-If Zvl32b is in effect the i64 and f64 RVV intrinsics need to be disabled.

Those rule are unclear to me too, I created an issue on vector spec, I guess we need writing few special rule for those extensions...

https://github.com/riscv/riscv-v-spec/issues/723

I think we need to visit some larger aspects of our vector implementation. Here are some thoughts.

-Most uses of Subtarget.hasStdExtV() don't really mean what the spec calls the standard V extension. They just means that we have vector instructions. Could be V, could be one of the Zve32* or Zve64* extensions.
-V extension passed to -march should imply at least Zvl128b.
-V extension passed to -march should enable F and D.
-Does Zvl32b passed to march enable vector instructions? Or do we still need Zve32* or Zve64* or V?
-If Zvl32b is in effect the i64 and f64 RVV intrinsics need to be disabled.

HsiangKai added inline comments.Aug 26 2021, 12:42 AM
llvm/lib/Target/RISCV/RISCVSubtarget.cpp
136

We have an implicit assumption that VLEN >= 64 from our VLA types. I suggest use 64 as the lower bound and add a FIXME here for VLEN is 32.

143

Should it be RVVVectorBitsMin < ZvlLen?

I suggest to return 0 and print a warning message for users.

HsiangKai added inline comments.Aug 26 2021, 12:55 AM
llvm/lib/Target/RISCV/RISCVSubtarget.cpp
118–121

Should it be less than or equal to ZvlLen? I would suggest to return 0 when RVVVectorBitsMax > ZvlLen.
The value also needs to >= 64.

craig.topper added inline comments.Aug 26 2021, 1:08 AM
llvm/lib/Target/RISCV/RISCVSubtarget.cpp
143

This isn't a good place to print warnings. It would just be a random message printed to stderr without going through any of clang's diagnostic infrastructure.

I think we need to visit some larger aspects of our vector implementation. Here are some thoughts.

-Most uses of Subtarget.hasStdExtV() don't really mean what the spec calls the standard V extension. They just means that we have vector instructions. Could be V, could be one of the Zve32* or Zve64* extensions.
-V extension passed to -march should imply at least Zvl128b.
-V extension passed to -march should enable F and D.
-Does Zvl32b passed to march enable vector instructions? Or do we still need Zve32* or Zve64* or V?
-If Zvl32b is in effect the i64 and f64 RVV intrinsics need to be disabled.

We need to apply the constraints of Zve* to intrinsics/pseudo instructions. I am not sure what is the behavior if users specify V and Zve* at the same time? I think Zve* is more complicated. We could prepare another patch for it.

Could we support Zvl32b? We already have an implicit constraint of vlen >= 64 from our VLA types, right?

eopXD added inline comments.Aug 26 2021, 1:11 AM
llvm/lib/Target/RISCV/RISCVSubtarget.cpp
143

I think moving the check to initializeSubtargetDependencies and use report_fatal_error will be a more appropriate approach?

craig.topper added inline comments.Aug 26 2021, 1:13 AM
llvm/lib/Target/RISCV/RISCVSubtarget.cpp
143

I think that causes clang to generate a crash report and telling the user to file a bug.

Forgot to mention, all this code is also being modified by D107290

eopXD added inline comments.Aug 26 2021, 1:20 AM
llvm/lib/Target/RISCV/RISCVSubtarget.cpp
143

I see, then my proposal is invalid.

I am not yet that familiar with the code base. I see RISCVFrameLowering.cpp using LLVMContext::diagnose, but RISCVSubtarget here doesn't seem to have access to LLVMContext.

May you recommend a pointer to some diagnose function I can use to tell the user this is an invalid setting between RVVVectorBitsMin(from riscv-v-vector-bits-min) and ZvlLen (from -march=zvl*b)?

I also don't think we're ready/able to support Zvl32b for the reasons stated above. I think the smoothest path forward would be, as @HsiangKai suggests, to only support VLEN>=64 and revisit 32. Supporting 32 is (I think) a much larger thing owing to our codegen assumptions on VLEN and ELEN. And, as @craig.topper said, there are a few little things that would probably be best prepared before this patch, replacing some of the calls to hasStdExtV at least (hasVVectors, for codegen purposes?). It sounds like we're unsure on some of the finer details about how the extensions interact.

I also don't think we're ready/able to support Zvl32b for the reasons stated above. I think the smoothest path forward would be, as @HsiangKai suggests, to only support VLEN>=64 and revisit 32. Supporting 32 is (I think) a much larger thing owing to our codegen assumptions on VLEN and ELEN. And, as @craig.topper said, there are a few little things that would probably be best prepared before this patch, replacing some of the calls to hasStdExtV at least (hasVVectors, for codegen purposes?). It sounds like we're unsure on some of the finer details about how the extensions interact.

I think our mapping from lmul/sew to <vscale x Y x iZ> types also break with VLEN=32. I haven't been able to figure out a single mapping that works for VLEN=32 and VLEN=64 and supports i64 vectors. To support VLEN=32 we need to reduce RVVBitsPerBlock to 32, but then you can't fit an i64 element into it.

khchen added a subscriber: khchen.Sep 1 2021, 12:47 AM
eopXD updated this revision to Diff 381737.Oct 23 2021, 6:37 AM

Address comments and add macro in clang.

eopXD retitled this revision from [RISCV] Add the zvl extension according to the v1.0-rc1 spec to [WIP][RISCV] Add the zvl extension according to the v1.0-rc2 spec.Oct 23 2021, 8:46 AM
eopXD edited the summary of this revision. (Show Details)
eopXD updated this revision to Diff 381747.Oct 23 2021, 9:13 AM

Rebase.

eopXD updated this revision to Diff 382543.Oct 27 2021, 12:59 AM

Fix clang-format.

eopXD updated this revision to Diff 382544.Oct 27 2021, 1:01 AM

Update code.

eopXD edited the summary of this revision. (Show Details)Oct 27 2021, 1:11 AM
eopXD updated this revision to Diff 382571.Oct 27 2021, 2:31 AM

Add implication: v imply d and f

eopXD updated this revision to Diff 382574.Oct 27 2021, 2:40 AM

Address comments:

  • remove duplicate checkDependency()
  • stay consistent in my patch of using auto for StringRef
eopXD updated this revision to Diff 382582.Oct 27 2021, 2:56 AM

Update.

eopXD updated this revision to Diff 382628.Oct 27 2021, 5:50 AM
eopXD marked 4 inline comments as done.

Change:

  • Address commments on restrictions in RISCVSubtarget.cpp
  • Fix clang-format fail
  • Fix test case fail
eopXD added inline comments.Oct 27 2021, 5:53 AM
llvm/lib/Target/RISCV/RISCVSubtarget.cpp
143

I see code under RISCVSubtarget::initializeSubtargetDependencies also using report_fatal_error:

eopXD marked an inline comment as not done.Oct 27 2021, 6:01 AM
eopXD updated this revision to Diff 382879.Oct 27 2021, 6:45 PM

Update code.

eopXD updated this revision to Diff 382880.Oct 27 2021, 6:48 PM

Update code.

llvm/lib/Target/RISCV/RISCVSubtarget.cpp
118–121

@HsiangKai I think you mis-typed the equation? It should be RVVVectorBitsMax < ZvlLen ;)

eopXD updated this revision to Diff 383599.Sat, Oct 30, 9:48 AM

Fix test case.

eopXD updated this revision to Diff 383601.Sat, Oct 30, 10:12 AM

Rebase to newest main.

eopXD updated this revision to Diff 383952.Mon, Nov 1, 7:59 PM

Rebase.

eopXD retitled this revision from [WIP][RISCV] Add the zvl extension according to the v1.0-rc2 spec to [RISCV] Add the zvl 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 zvl extension according to the v1.0-rc2 spec to [RISCV] Add the zvl extension according to the v1.0 spec.Tue, Nov 2, 1:29 AM
eopXD edited the summary of this revision. (Show Details)