This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][Driver] Add -mrvv-vector-bits= option similar to -msve-vector-bits=
ClosedPublic

Authored by craig.topper on Jan 19 2023, 10:49 AM.

Details

Summary

This option will control the vscale min/max.

I have left out the '+' support that SVE supports for now. We already
have minimum controlled by the Zvl*b extension so this didn't seem that
useful.

I've added "scalable" from SVE to allow the option to be cancelled later on
command line. Though this name might make less sense for RISC-V since
the word "scalable" does not appear in the V spec. Maybe something like
"unknown" or "runtime" or "variable" would be better?

In addition to "scalable", 64, 128, 256, 512, ..., 65536, I have added an extra
value "zvl" that will use the value from Zvl*b as the min and max.
This avoids repeating the numeric value in two places or to get
min/max from -mcpu.

The primary effect of this option today is simplification of stack
address calculations for RVV vectors and avoiding the use of
vrgatherei16 in some cases if we know there are less than 256 elements.

Future patches may add something similar to the arm_sve_vector_bits
attribute to allow RVV vectors to be used in structs and global
variables.

Diff Detail

Event Timeline

craig.topper created this revision.Jan 19 2023, 10:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2023, 10:49 AM
craig.topper requested review of this revision.Jan 19 2023, 10:49 AM
Matt added a subscriber: Matt.Jan 19 2023, 9:50 PM
frasercrmck added inline comments.Jan 23 2023, 1:48 AM
clang/include/clang/Driver/Options.td
3591

Same question as below with the value "scalable". Should we excise it for now?

clang/lib/Driver/ToolChains/Clang.cpp
2150

Is this check right? I don't see mention of "scalable" in the commit description. Should this be a plain else?

clang/test/Driver/riscv-rvv-vector-bits.c
3

Need to adjust this comment

17

Another use of "scalable" which I'm not sure about.

craig.topper added inline comments.Jan 23 2023, 9:35 AM
clang/lib/Driver/ToolChains/Clang.cpp
2150

"scalable" allows you to cancel the option appearing earlier on the command line. Similar to why most bool options have a "no-" version. It's let you do thing like append to CFLAGS to override a global setting in a make file.

craig.topper edited the summary of this revision. (Show Details)Jan 23 2023, 10:02 AM
MaskRay added inline comments.Jan 23 2023, 11:02 AM
clang/docs/ReleaseNotes.rst
160
clang/include/clang/Driver/Options.td
3592

Conform to the majority style, not the few exceptions

clang/test/Driver/riscv-rvv-vector-bits.c
6

--target=

-target is legacy.

craig.topper retitled this revision from [RISCV][Driver] Add -rvv-vector-bits= option similar to -sve-vector-bits= to [RISCV][Driver] Add -mrvv-vector-bits= option similar to -msve-vector-bits=.Jan 23 2023, 5:30 PM
frasercrmck added inline comments.Jan 24 2023, 8:06 AM
clang/lib/Driver/ToolChains/Clang.cpp
2150

Makes sense, thanks. I'm happy to include it as an option value, now that it's documented.

Generally supportive of having such an option, but going to defer to others on the review. I don't work enough on clang to have an opinion on code here.

clang/docs/ReleaseNotes.rst
160

Correct me if I'm wrong, but doesn't this set both an upper and lower bound? If so, the wording in the note here needs changed. If not, the naming should probably be something like rvv-vector-bits-max.

Address review comment in release notes.

frasercrmck accepted this revision.Feb 2 2023, 2:05 AM

LGTM other than test header comment that needs changed.

clang/test/Driver/riscv-rvv-vector-bits.c
3

Still need to adjust this comment

This revision is now accepted and ready to land.Feb 2 2023, 2:05 AM
This revision was landed with ongoing or failed builds.Feb 2 2023, 10:32 AM
This revision was automatically updated to reflect the committed changes.
wangpc added a subscriber: wangpc.Aug 21 2023, 3:13 AM
wangpc added inline comments.
clang/test/Driver/riscv-rvv-vector-bits.c
43

Why isn't 64 an valid value?

craig.topper added inline comments.Aug 21 2023, 8:56 AM
clang/test/Driver/riscv-rvv-vector-bits.c
43

Because the command line contains ‘v’, zvl128b is implied. So rvv-vector-bits must be at least 128.

evandro removed a subscriber: evandro.Aug 21 2023, 12:28 PM