This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add attribute(riscv_rvv_vector_bits(N)) based on AArch64 arm_sve_vector_bits.
ClosedPublic

Authored by craig.topper on Mar 1 2023, 10:49 AM.

Details

Summary

This allows the user to set the size of the scalable vector so they
can be used in structs and as the type of global variables. This works
by representing the type as a fixed vector instead of a scalable vector
in IR. Conversions to and from scalable vectors are made where necessary
like function arguments/returns and intrinsics.

This features has been requested here
https://github.com/riscv-non-isa/rvv-intrinsic-doc/issues/176
I know arm_sve_vector_bits is used by the Eigen library so this
could be used to port Eigen to RVV.

This patch adds a new preprocessor define __riscv_v_fixed_vlen that
is set when -mrvv_vector_bits is passed on the command line.

The code is largely based on the AArch64 code. A lot of code was
copy/pasted and then modiied to RVV. There may be some opportunities
for sharing.

This first patch only supports the LMUL=1 types. Additional changes
will be needed to support other LMULs. I have also not supported
mask vectors.

Diff Detail

Event Timeline

craig.topper created this revision.Mar 1 2023, 10:49 AM
Herald added a project: Restricted Project. · View Herald Transcript
craig.topper requested review of this revision.Mar 1 2023, 10:49 AM
Matt added a subscriber: Matt.Mar 1 2023, 1:03 PM

Add more operator tests.

Add name mangling and type info tests.

craig.topper retitled this revision from [RISCV][WIP] Add attribute(riscv_rvv_vector_bits(N)) based on AArch64 arm_sve_vector_bits. to [RISCV] Add attribute(riscv_rvv_vector_bits(N)) based on AArch64 arm_sve_vector_bits..
craig.topper edited the summary of this revision. (Show Details)
Any binary that uses this feature is not forward portable to hardware
with a larger vector size. That's true for SVE as well.

I did not understood this sentence. AFAIK, SVE uses the ptrue instruction to generate a mask to only activate the necessary lanes. If I do fixed length SVE with 128 bit and you give a machine 2048 bits, then it should still work. Probably I missed something.

craig.topper edited the summary of this revision. (Show Details)Mar 2 2023, 11:04 AM
Any binary that uses this feature is not forward portable to hardware
with a larger vector size. That's true for SVE as well.

I did not understood this sentence. AFAIK, SVE uses the ptrue instruction to generate a mask to only activate the necessary lanes. If I do fixed length SVE with 128 bit and you give a machine 2048 bits, then it should still work. Probably I missed something.

Maybe I'm wrong, but there are few statements here https://developer.arm.com/documentation/101726/0400/Coding-for-Scalable-Vector-Extension--SVE-/SVE-Vector-Length-Specific--VLS--programming

"When you implement your code, you can choose to use fixed-length vectors. Fixed-length vectors enable the use of constructs that are generally not safe for code which is to be run on targets with unknown SVE vector lengths. However, if you do not require your code to be portable, VLS code can be more optimal than VLA code for a specific SVE implementation."

"Generated VLS code must only be executed on hardware which offers an SVE vector length compatible with the intent of the programmer. "

aaron.ballman added inline comments.Mar 15 2023, 7:46 AM
clang/include/clang/Basic/AttrDocs.td
2344

You should add some details about requirements on the argument to the attribute (like the range of valid values, that it needs to be a power-of-two value, etc) and what happens when you write the attribute on a non-sizeless type.

clang/include/clang/Basic/DiagnosticSemaKinds.td
3062
clang/lib/AST/ItaniumMangle.cpp
3903–3905

Should there be corresponding changes to the Microsoft mangler as well?

3932–3934

Same here.

clang/lib/AST/TypePrinter.cpp
703–706

Bummer we don't have an ASTContext handy so we could call getTypeSizeInChars()...

craig.topper added inline comments.Mar 15 2023, 2:37 PM
clang/lib/AST/ItaniumMangle.cpp
3903–3905

Good question. I don't see the equivalent SVE handling in the Microsoft mangler.

Address comments other than Microsoft mangler

aaron.ballman added inline comments.Mar 16 2023, 5:22 AM
clang/lib/AST/ItaniumMangle.cpp
3903–3905

I'm fine if you want to address that issue in a follow-up.

Adding Erich as attributes code owner and John/Eli as ABI code owners.

clang/lib/Sema/SemaType.cpp
8336

Should this be done as part of this patch (are we accepting code we shouldn't be accepting)?

craig.topper added inline comments.Apr 3 2023, 9:22 AM
clang/lib/Sema/SemaType.cpp
8305

I need to fix this comment.

8336

No. I need to phrase this FIXME better. I'm only accepting types that have LMUL=1. (length multiplier). This is enforced in Type::isRVVVLSBuiltinType() where there's another FIXME about LMUL=1.

Fix some comments

So I don't see any handling of the dependent version of this, we probably need tests for those at minimum.

erichkeane added inline comments.Apr 11 2023, 11:26 AM
clang/docs/ReleaseNotes.rst
393

Would love it if we defined "RVV" here.

clang/include/clang/AST/ASTContext.h
2262

Same here, what is 'lax compatible' mean here? And RVV?

clang/lib/CodeGen/TargetInfo.cpp
11370

I wonder if at least the inner type can be picked up ConvertType instead. There doesn't seem to be obvious rhyme/reason to the last argument to ScalableVectorType, so it might not solve that.

However, it'll solve the long problem.

11391

Where is 'XLen' from here?

craig.topper added inline comments.Apr 11 2023, 1:24 PM
clang/include/clang/AST/ASTContext.h
2262

Do you have the same comment for the AArch64 equivalent on line 2252?

clang/lib/CodeGen/TargetInfo.cpp
11370

The last argument is 64 / sizeof(element). I should replace the 64 with RISCV::RVVBitsPerBlock.

11372

I need to fix this SVE usage here.

11391

It's a member of RISCVABIInfo. It's 64 for riscv64 triple and 32 for riscv32 triple.

has this had an RFC btw? I don't believe I've seen one, and this looks like we probably need one.

clang/include/clang/AST/ASTContext.h
2262

I do NOW!

clang/lib/CodeGen/TargetInfo.cpp
11370

In that case, I'd suggest putting the 'math' in here. It would be much less difficult to read the tea leaves.

11391

Well, the name is awful :) I'd probably suggest a re-name and hiding it behind a function call (since that way it can be done on the triple, rather than an initialized variable perhaps?), but I'm not really in charge of this target info.

jrtc27 added inline comments.Apr 11 2023, 1:32 PM
clang/lib/CodeGen/TargetInfo.cpp
11391

It's not for anyone in the RISC-V space, since it is defined by the architecture and used pervasively (and means the X register LENgth, i.e. how many bits in the x0-x31 GPRs). Using anything else in a RISC-V ABI context would be worse from a RISC-V perspective. In a random LLVM checkout I have I see 1118 instances of /xlen/i in llvm/lib/Target/RISCV alone.

has this had an RFC btw? I don't believe I've seen one, and this looks like we probably need one.

It has not had an RFC. It's almost a direct copy of AArch64's implementation, but changed for RISC-V. Do you know if there was an RFC for AArch64?

clang/lib/CodeGen/TargetInfo.cpp
11391

It's the term in the RISC-V spec for the size of our integer registers. Anyone working on RISC-V should be familiar with it.

has this had an RFC btw? I don't believe I've seen one, and this looks like we probably need one.

It has not had an RFC. It's almost a direct copy of AArch64's implementation, but changed for RISC-V. Do you know if there was an RFC for AArch64?

There was for SVE, is that what you mean? I believe most of that went through extensive RFC.

The Sema & before stuff seems fine to me, CodeGen is owned by others, so it'll be up to them. I'm not super up on what RFCs happened/were required for this for AArch64, but I'd suggest we at least have the implementers of the AArch64 implementation review this as well.

clang/lib/CodeGen/TargetInfo.cpp
11391

Based on Jessica's post, perhaps it is not an issue. Just was jarring to see something as impenetrable. I'd perhaps suggest something like XRegisterLen to make it clear what 'X' is, but just a suggestion for the next folks who are finding their way into contributing patches, despite perhaps not yet being RISCV experts.

So I don't see any handling of the dependent version of this, we probably need tests for those at minimum.

Does SVE handle the dependent version?

Address review comments

craig.topper added inline comments.Apr 11 2023, 3:15 PM
clang/include/clang/AST/ASTContext.h
2262

I mentioned -flax-vector-conversions=. Is that sufficient?

clang/lib/CodeGen/TargetInfo.cpp
11391

I rewrote it using ConvertType and getTypeSize.

So I don't see any handling of the dependent version of this, we probably need tests for those at minimum.

Does SVE handle the dependent version?

It does, I believe we insisted on it at the time. You may inherit it sufficiently, so tests for it are perhaps all that is necessary.

clang/include/clang/AST/ASTContext.h
2262

I still had to look this one up.

clang/lib/CodeGen/TargetInfo.cpp
11371

Having the switch still is awkward, since it only exists for an unreachable. I wonder if splitting off this type checking to a separate function and asserting on it is more valuable? AND could be used elsewhere if we use this pattern again?

I'll leave that up to the CodeGen code owners to require however.

craig.topper added inline comments.Apr 12 2023, 8:56 AM
clang/test/SemaCXX/attr-riscv-rvv-vector-bits.cpp
12

@erichkeane does this cover the dependent case or were you looking for something else?

Here are on the only mentions of template I see in SVE tests that use this attribute.

clang/test$ ack template `ack arm_sve_vector -l`
CodeGenCXX/aarch64-mangle-sve-fixed-vectors.cpp
37:template <typename T> struct S {};

SemaCXX/attr-arm-sve-vector-bits.cpp
16:template<typename T> struct S { T var; };

Here is the result for this patch

clang/test$ ack template `ack riscv_rvv_vector -l`
CodeGenCXX/riscv-mangle-rvv-fixed-vectors.cpp
48:template <typename T> struct S {};

SemaCXX/attr-riscv-rvv-vector-bits.cpp
12:template<typename T> struct S { T var; };
craig.topper added inline comments.Apr 12 2023, 9:00 AM
clang/include/clang/AST/ASTContext.h
2262

That's not quite the description of -flax-vector-conversion. The total vector size must be the same. But the element size and number of elements can be different.

Added description of -flax-vector-conversions taken from gcc's description.

erichkeane added inline comments.Apr 12 2023, 9:08 AM
clang/include/clang/AST/ASTContext.h
2263
clang/test/SemaCXX/attr-riscv-rvv-vector-bits.cpp
12

Thats unfortunate, and I wish I'd thought of it at the time/been more active reviewing the SVE stuff then. Really what I'm looking for is:

template<int N> 
struct Whatever {
  using Something = char __attribute((riscv_rvv_vector_bits(N)));
};

void Func(Whatever<5>::Something MyVar){}
craig.topper added inline comments.Apr 12 2023, 9:48 AM
clang/test/SemaCXX/attr-riscv-rvv-vector-bits.cpp
12

That does not appear to work.

$ ./bin/clang test.cpp --target=riscv64 -march=rv64gcv -mrvv-vector-bits=zvl
test.cpp:3:41: error: 'riscv_rvv_vector_bits' attribute requires an integer constant
    using Something = char __attribute((riscv_rvv_vector_bits(N)));

It's not very useful as a template parameter. There's only one value that works and that's whatever __RISCV_RVV_VLEN_BITS is set to.

craig.topper marked an inline comment as done.Apr 12 2023, 9:57 AM
erichkeane added inline comments.Apr 12 2023, 9:57 AM
clang/test/SemaCXX/attr-riscv-rvv-vector-bits.cpp
12

Thats really unfortunate, but it makes me wonder what DependentVectorType is for in this case, or the handling of said things. Because I would expect:

template<typename T, int Size>
using RiscvVector = T __attribute__((risv_rvv_vector_bits(Size)));

RiscvVector<char, <TheRightAnswer>> Foo;

to be useful. Even if not, I'd expect:

template<typename T>
using RiscvVector = T __attribute__((risv_rvv_vector_bits(TheRightAnswer)));
RiscvVector<char> Foo;

to both work.

It's not very useful as a template parameter. There's only one value that works and that's whatever __RISCV_RVV_VLEN_BITS is set to.

This makes me wonder why this attribute takes an integer constant anyway, if it is just a 'guess what the right answer is!' sorta thing. Seems to me this never should have taken a parameter.

aaron.ballman added inline comments.Apr 12 2023, 10:11 AM
clang/test/SemaCXX/attr-riscv-rvv-vector-bits.cpp
12

It's not very useful as a template parameter. There's only one value that works and that's whatever __RISCV_RVV_VLEN_BITS is set to.

Can you help me understand why the argument exists then?

We're pretty inconsistent about attribute arguments properly handling things like constant expressions vs integer literals, but the trend lately is to accept a constant expression rather than only a literal because of how often users like to give names to literals and how much more constexpr code we're seeing in the wild.

craig.topper added inline comments.Apr 12 2023, 10:29 AM
clang/test/SemaCXX/attr-riscv-rvv-vector-bits.cpp
12

This is what's in ARM's ACLE documentation:

The ACLE only defines the effect of the attribute if all of the following are true:

  1. the attribute is attached to a single SVE vector type (such as svint32_t) or to the SVE predicate

type svbool_t;

  1. the arguments “…” consist of a single nonzero integer constant expression (referred to as N below); and
  2. N==__ARM_FEATURE_SVE_BITS.

In other cases the implementation must do one of the following:
• ignore the attribute; a warning would then be appropriate, but is not required
• reject the program with a diagnostic
• extend requirement (3) above to support other values of N besides __ARM_FEATURE_SVE_BITS
• process the attribute in accordance with a later revision of the ACLE

So there's a bullet in there that allows an implementation to support other values, but it is not required.

aaron.ballman added inline comments.Apr 12 2023, 11:55 AM
clang/test/SemaCXX/attr-riscv-rvv-vector-bits.cpp
12

Thank you, the current design makes more sense to me now. I'm less concerned about whether we support dependent values for this attribute argument. If we start to support values of N other than __ARM_FEATURE_SVE_BITS then it might make sense to care about it at that point. But I don't think users are going to do stuff like:

template <int N>
using fixed_int8m1_t __attribute__((riscv_rvv_vector_bits(N))) = vint8m1_t;

fixed_int8m1_t<__ARM_FEATURE_SVE_BITS> foo;

However, it is still important to test that the type attribute works in a situation like:

template <typename Ty>
using Something = Ty __attribute__((riscv_rvv_vector_bits(__ARM_FEATURE_SVE_BITS)));

// Ensure that Something is correctly attributed, that the underlying type for Ty is valid for the attribute, etc
craig.topper added inline comments.Apr 13 2023, 11:10 AM
clang/test/SemaCXX/attr-riscv-rvv-vector-bits.cpp
12

It looks like it doesn't work for that case.

erichkeane added inline comments.Apr 13 2023, 11:15 AM
clang/test/SemaCXX/attr-riscv-rvv-vector-bits.cpp
12

THAT is super unfortunate, and really should work in this case. The SVE implementers could probably help out here.

craig.topper added inline comments.Apr 13 2023, 7:24 PM
clang/test/SemaCXX/attr-riscv-rvv-vector-bits.cpp
12

Is that blocking for this patch?

aaron.ballman added inline comments.Apr 14 2023, 4:31 AM
clang/test/SemaCXX/attr-riscv-rvv-vector-bits.cpp
12

It's @erichkeane 's call, but personally, I don't think that should block this patch (only because it's a second instance of an existing issue and this patch is quite large already, basically), but it definitely needs to be solved here and for SVE rather than kicking the can down the road to someone else. New types need to fit into the type system cleanly and that includes being able to use them from templates.

So how about this for a compromise: file an issue (or more than one if you'd prefer) to fix these attributed types up so we don't forget to do it, and plan to work on that issue ASAP (or rope someone else into it).

This patch LGTM given the above compromise, but one of the clang-codegen needs to take a look to accept.

clang/test/SemaCXX/attr-riscv-rvv-vector-bits.cpp
12

So how about this for a compromise: file an issue (or more than one if you'd prefer) to fix these attributed types up so we don't forget to do it, and plan to work on that issue ASAP (or rope someone else into it).

I think this is an acceptable compromise to me.

The CodeGen change looks fine. I'm surprised you didn't need any code in argument/parameter/call/return emission to do the actual fixed<->scalable coercion; do we already have that for other reasons?

clang/include/clang/Basic/AttrDocs.td
2332

This probably needs a defined(__RISCV_RVV_VLEN_BITS) clause, right? Because the compiler doesn't actually define this macro unless -mrvv-vector-bits is given.

2337
2345

This doesn't describe the actual behavior of the compiler, which is that it's *ill-formed* to use this attribute except when providing the same value to -mrvv-vector-bits.

Also, this feels like an awkward attempt to also document the __RISCV_RVV_VLEN_BITS macro, which probably ought to be primarily documented in the command line argument reference for -mrvv-vector-bits.

2347
clang/lib/Basic/Targets/RISCV.cpp
207

Is this macro name coming from somewhere specifically? Because it doesn't match the normal scheme for RISC-V target macros, which are all lowercase, and it doesn't match the name of the command line argument it reflects.

Also, why is the computation of this thing so complicated when the command-line argument is basically a single number?

craig.topper added inline comments.Apr 18 2023, 12:54 AM
clang/include/clang/Basic/AttrDocs.td
2332

I guess so. I copied the documentation from the SVE attribute and modified it to RISC-V.

2345

This doesn't describe the actual behavior of the compiler, which is that it's *ill-formed* to use this attribute except when providing the same value to -mrvv-vector-bits.

I think that means the SVE doc is also incorrect?

Also, this feels like an awkward attempt to also document the __RISCV_RVV_VLEN_BITS macro, which probably ought to be primarily documented in the command line argument reference for -mrvv-vector-bits.

Ok I'll move it there.

clang/lib/Basic/Targets/RISCV.cpp
207

Is this macro name coming from somewhere specifically? Because it doesn't match the normal scheme for RISC-V target macros, which are all lowercase, and it doesn't match the name of the command line argument it reflects.

I made it up. I'll reconsider it.

Also, why is the computation of this thing so complicated when the command-line argument is basically a single number?

The command line is converted to -mvscale-min= and -mvscale-max= options just like SVE. We divide by llvm::RISCV::RVVBitsPerBlock where SVE divides by 128.

RISC-V does have a concept of minimum vector length through -march already which is checked by getVScaleRange to deal with any disagreement. There's a special value -mriscv-rvv-vector-bits=zvl to use the minimum value from -march without needing to repeat the value.

The CodeGen change looks fine. I'm surprised you didn't need any code in argument/parameter/call/return emission to do the actual fixed<->scalable coercion; do we already have that for other reasons?

You mean RISC-V specific code or generic code? If generic, I assume we got it from SVE's earlier implementation.

You mean RISC-V specific code or generic code? If generic, I assume we got it from SVE's earlier implementation.

Ah, if SVE has a similar feature then that makes sense.

clang/include/clang/Basic/AttrDocs.td
2332

Ah, I see. Yeah, it's probably wrong there, too.

2345

I think that means the SVE doc is also incorrect?

Yeah.

clang/lib/Basic/Targets/RISCV.cpp
207

Okay. So in principle this could be extended to something like a rule where we statically check only that the value is within the specified range, and then it would be dynamically UB to use a type that's wrong for the actual runtime processor? Maybe that was the idea with SVE but it just never got implemented, which is why the documentation looks the way it does.

craig.topper added inline comments.Apr 18 2023, 6:46 PM
clang/include/clang/Basic/AttrDocs.td
2332

As written the #if would evaluate to false if __RISCV_RVV_VLEN_BITS isn't defined or it's not defined to be 512. The code line it was guarding is using a hardcoded 512.

This isn't how I'd encourage this to be used so I'm changing to #if defined() and will use the preprocessor define in the next line.

Renamed preprocessor define to riscv_v_fixed_vlen. This makes it similar to the existing riscv_v_min_vlen.

craig.topper edited the summary of this revision. (Show Details)Apr 19 2023, 10:57 AM

Thanks, a couple very minor fixes / requests, but feel free to commit afterwards.

clang/include/clang/Basic/AttrDocs.td
2323

Maybe this is obvious from the attribute name, but it's better to be clear.

2332

Address review comments

This revision was not accepted when it landed; it landed in state Needs Review.Apr 28 2023, 3:41 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

Hi @craig.topper , this patch is causing a build failure:

In file included from /llvm-project/clang/lib/Sema/SemaType.cpp:43:
/llvm-project/llvm/include/llvm/TargetParser/RISCVTargetParser.h:32:10: fatal error: 'llvm/TargetParser/RISCVTargetParserDef.inc' file not found

To reproduce, configure from a clean build directory like this:

cmake -G Ninja /path/to/llvm-project/llvm \
  -DLLVM_TARGETS_TO_BUILD="X86;ARM;AArch64" \
  -DCMAKE_BUILD_TYPE:STRING=Release \
  -DLLVM_ENABLE_PROJECTS="clang"

Then run:

ninja tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaType.cpp.o

Could you take a look? 🙏

Hi @craig.topper , this patch is causing a build failure:

In file included from /llvm-project/clang/lib/Sema/SemaType.cpp:43:
/llvm-project/llvm/include/llvm/TargetParser/RISCVTargetParser.h:32:10: fatal error: 'llvm/TargetParser/RISCVTargetParserDef.inc' file not found

To reproduce, configure from a clean build directory like this:

cmake -G Ninja /path/to/llvm-project/llvm \
  -DLLVM_TARGETS_TO_BUILD="X86;ARM;AArch64" \
  -DCMAKE_BUILD_TYPE:STRING=Release \
  -DLLVM_ENABLE_PROJECTS="clang"

Then run:

ninja tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaType.cpp.o

Could you take a look? 🙏

Thanks, I pushed a4797869e73355209206a5175c11bedb14013211 to fix this. I'm going to find a better home for RISCV::RVVBitsPerBlock so we can remove this dependency.

CodeGen has the same issue:

$ ninja tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/TargetInfo.cpp.o
In file included from /llvm-project/clang/lib/CodeGen/TargetInfo.cpp:36:
/llvm-project/llvm/include/llvm/TargetParser/RISCVTargetParser.h:32:10: fatal error: 'llvm/TargetParser/RISCVTargetParserDef.inc' file not found

CodeGen has the same issue:

$ ninja tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/TargetInfo.cpp.o
In file included from /llvm-project/clang/lib/CodeGen/TargetInfo.cpp:36:
/llvm-project/llvm/include/llvm/TargetParser/RISCVTargetParser.h:32:10: fatal error: 'llvm/TargetParser/RISCVTargetParserDef.inc' file not found

I just posted https://reviews.llvm.org/D149606 to move the constant to a file that doesn't depend on tablegen.

CodeGen has the same issue:

$ ninja tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/TargetInfo.cpp.o
In file included from /llvm-project/clang/lib/CodeGen/TargetInfo.cpp:36:
/llvm-project/llvm/include/llvm/TargetParser/RISCVTargetParser.h:32:10: fatal error: 'llvm/TargetParser/RISCVTargetParserDef.inc' file not found

I just posted https://reviews.llvm.org/D149606 to move the constant to a file that doesn't depend on tablegen.

I found an even better fix fa42e7b