This is an archive of the discontinued LLVM Phabricator instance.

[Clang][RISCV] Define RISC-V V builtin types
ClosedPublic

Authored by HsiangKai on Dec 4 2020, 9:21 PM.

Details

Summary

Add the types for the RISC-V V extension builtins.

These types will be used by the RISC-V V intrinsics which require types of the form <vscale x 1 x i64>(LMUL=1 element size=64) or <vscale x 4 x i32>(LMUL=2 element size=32), etc. The vector_size attribute does not work for us as it doesn't create a scalable vector type. We want these types to be opaque and have no operators defined for them. We want them to be sizeless. This makes them similar to the ARM SVE builtin types. But we will have quite a bit more types. This patch adds around 60. Later patches will add another 230 or so types representing tuples of these types similar to the x2/x3/x4 types in ARM SVE. But with extra complexity that these types are combined with the LMUL concept that is unique to RISCV.

For more background see this RFC http://lists.llvm.org/pipermail/llvm-dev/2020-October/145850.html

Authored-by: Hsiangkai Wang <kai.wang@sifive.com>

Diff Detail

Event Timeline

evandro created this revision.Dec 4 2020, 9:21 PM
evandro requested review of this revision.Dec 4 2020, 9:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 4 2020, 9:21 PM

Can you re-upload the patch with full context?

khchen added a subscriber: khchen.Dec 5 2020, 6:07 AM
HsiangKai edited reviewers, added: HsiangKai; removed: Hsiang-Kai.Dec 5 2020, 6:53 AM
evandro updated this revision to Diff 309749.Dec 5 2020, 2:12 PM
evandro updated this revision to Diff 309750.Dec 5 2020, 2:16 PM
craig.topper added inline comments.Dec 5 2020, 2:44 PM
clang/include/clang/Basic/RISCVVTypes.def
33

NF argument isn't documented. And is always 1.

clang/include/clang/Basic/TargetInfo.h
221

This needs to be initialized to false in TargetInfo::TargetInfo() in TargetInfo.cpp I think. And then set to true from lib/Basic/Targets/RISCV.cpp

clang/lib/AST/ASTContext.cpp
2185

Does this alignment need to be this high? The VMV0 register class in the backend has an alignment of 64 for spills. Just wondering why they aren't consistent.

clang/lib/CodeGen/CGDebugInfo.cpp
779

Is there a function name in DwarfUnt.cpp that would be a better reference here? Line numbers aren't stable.

HsiangKai added inline comments.Dec 5 2020, 6:39 PM
clang/include/clang/Basic/RISCVVTypes.def
33

NF argument isn't documented. And is always 1.

We could remove it in this patch. The field will be needed when we are going to upstream Zvlsseg implementation.

clang/lib/AST/ASTContext.cpp
2185

Does this alignment need to be this high? The VMV0 register class in the backend has an alignment of 64 for spills. Just wondering why they aren't consistent.

Indeed, it should be 64.

clang/lib/CodeGen/CGDebugInfo.cpp
779

Is there a function name in DwarfUnt.cpp that would be a better reference here? Line numbers aren't stable.

It should be hasVectorBeenPadded(). I have a downstream modification not upstreamed. We could remove the debug info for RVV types from this patch. I think it could be another patch for debug info for scalable vector types.

arcbbb added a subscriber: arcbbb.Dec 6 2020, 6:44 PM
xmj added a subscriber: xmj.Dec 6 2020, 7:20 PM
liaolucy added inline comments.
clang/include/clang/Basic/RISCVVTypes.def
68

RISC-V V has too many types, more than 200. All types use builtin types? Is it possible to reduce the number of builtin types?

jrtc27 added inline comments.Dec 7 2020, 3:33 AM
clang/include/clang/Basic/RISCVVTypes.def
68

Indeed this is madness, what's wrong with just using __attribute__((vector_size(n))) on the right type? We should not be encouraging people to write code with architecture-specific types... but if we _really_ need these because RISC-V GCC decided this is how RISC-V V is going to look them can we not just shove them all in a header as typedef's for the architecture-independent attributed types and push that complexity out of the compiler itself?

craig.topper added inline comments.Dec 7 2020, 9:37 AM
clang/include/clang/Basic/RISCVVTypes.def
68

We are using <vscale x 1 x i64> to specify types in IR. The size of the fixed part is being used to control the LMUL parameter. There is currently no way to spell a scalable vector type in C in a generic way.

Alternatively I guess we could make LMUL a parameter to the intrinsic and create the scalable IR types in the frontend based on it?

craig.topper added inline comments.Dec 7 2020, 10:04 AM
clang/include/clang/Basic/RISCVVTypes.def
68

I do wonder why we bothered to have signed and unsigned types. The signedness of the operation should be carried in the intrinsic name.

evandro marked 7 inline comments as done.Dec 7 2020, 3:36 PM
evandro added inline comments.
clang/include/clang/Basic/RISCVVTypes.def
33

Perhaps we can just note that NF is going to be used by Zvlsseg instead.

68

Some integer operations distinguish between signed and unsigned.

clang/lib/AST/ASTContext.cpp
2185

Does this alignment need to be this high? The VMV0 register class in the backend has an alignment of 64 for spills. Just wondering why they aren't consistent.

Indeed, it should be 64.

Good catch!

evandro updated this revision to Diff 310044.Dec 7 2020, 3:44 PM
evandro marked 2 inline comments as done.
evandro marked an inline comment as not done.Dec 7 2020, 3:49 PM
khchen added inline comments.Dec 7 2020, 7:32 PM
clang/include/clang/Basic/RISCVVTypes.def
68

I do wonder why we bothered to have signed and unsigned types. The signedness of the operation should be carried in the intrinsic name.

I think the only good thing for supporting both signed and unsigned type is that are more readable and compiler can does type checking for programmer.

maybe the alternative way is changing intrinsic naming rule like

vint32m1_t a, b, c;
a = vadd_i32m1(b, c);
vuint32m1_t a, b, c;
a = vadd_u32m1(b, c);
vfloat32m1_t a, b, c;
a = vadd_f32m1(b, c);
kito-cheng added inline comments.Dec 7 2020, 9:10 PM
clang/include/clang/Basic/RISCVVTypes.def
68

One quick thought about this, if the concern is too much built-in types are introduced in clang, maybe we could add a new attribute like __attribute__((vector_size(n))), maybe named __attribute__((riscv_scaleble_vector("[1|2|4|8|f2|f4|f8]")))? and use that to define vector types like typedef int __attribute__((riscv_scaleble_vector("2"))) vintm2_t.

HsiangKai added inline comments.Dec 8 2020, 12:25 AM
clang/include/clang/Basic/RISCVVTypes.def
68

To have signed and unsigned types, we could do type checking for the builtins. For example, we have vmaxu_vv and vmax_vv. Users could pass signed vector types into unsigned version vmaxu_vv if we only have types without signed/unsigned. How could we tell users they pass values into the wrong builtins?

liaolucy added inline comments.Dec 8 2020, 12:26 AM
clang/include/clang/Basic/RISCVVTypes.def
69

eg: typedef attribute((riscv_vector_type(16/*EIBits*/, 1/*LMUL*/, ...))) int16_t vint16m1_t;
Please help to consider if this is feasible.

craig.topper added inline comments.Dec 8 2020, 12:35 AM
clang/include/clang/Basic/RISCVVTypes.def
68

Maybe I've been corrupted by X86 where there is just a single 128-bit integer vector type. Even though there are 8, 16, 32, 64 bit element operations with some being signed/unsigned.

martong removed a subscriber: martong.Dec 8 2020, 6:12 AM

Can we discuss this patch in tomorrows RISC-V meeting? @jrtc27 @kito-cheng @khchen @liaolucy

craig.topper edited the summary of this revision. (Show Details)Dec 9 2020, 12:13 PM
craig.topper edited the summary of this revision. (Show Details)Dec 9 2020, 12:34 PM
asb added a comment.Dec 10 2020, 4:49 AM

Can we discuss this patch in tomorrows RISC-V meeting? @jrtc27 @kito-cheng @khchen @liaolucy

I've added it to the agenda

I think it would be possible to add a new attribute to define these types. But only specific values for parameters would be allowed so that it could only generate the exact types we see here and the future segment load patches. It wouldn't be general purpose like vector_size since the backend design has a contract about how to translate the LMUL value from the scalable type.

This would introduce a new type in the AST or at least new subtype of VectorType. This new type would need to be checked for in multiple places in the compiler to define its behavior. As a datapoint VectorType::SveFixedLengthDataVector appears 20 times. It's not clear that's a reduction in complexity versus following what was already done with AArch64SVEACLETypes.def. But I'm not a frontend expert.

@rsmith or @rjmccall do you have any opinions on whether we should use builtin types like SVE or create a new attribute for RISCV?

I think it would be possible to add a new attribute to define these types. But only specific values for parameters would be allowed so that it could only generate the exact types we see here and the future segment load patches. It wouldn't be general purpose like vector_size since the backend design has a contract about how to translate the LMUL value from the scalable type.

This would introduce a new type in the AST or at least new subtype of VectorType. This new type would need to be checked for in multiple places in the compiler to define its behavior. As a datapoint VectorType::SveFixedLengthDataVector appears 20 times. It's not clear that's a reduction in complexity versus following what was already done with AArch64SVEACLETypes.def. But I'm not a frontend expert.

FWIW, when I went to introduce the predecessor to _ExtInt, @rsmith was very against using an attribute spelling and wanted a declaration keyword (which is partly how _ExtInt ended up how it was).

I don't think it is controversial to be a new AST type however.

@liaolucy RISC-V vector types are sizeless types. Sizeless is kind of characteristic for builtin types. If we use attribute to declare RISC-V vector types, the frontend does not know anything about it. I still think to define RISC-V vector types as builtin types is a better way.

HsiangKai commandeered this revision.Jan 13 2021, 8:08 AM
HsiangKai edited reviewers, added: evandro; removed: HsiangKai.

@liaolucy RISC-V vector types are sizeless types. Sizeless is kind of characteristic for builtin types. If we use attribute to declare RISC-V vector types, the frontend does not know anything about it. I still think to define RISC-V vector types as builtin types is a better way.

Thanks, It's OK for me.

HsiangKai updated this revision to Diff 316553.Jan 13 2021, 7:22 PM

Refine debug info for RVV types.

HsiangKai updated this revision to Diff 316556.Jan 13 2021, 7:35 PM

Recover comments.

According to "9. Vector Memory Alignment Constraints" in V specification, change the alignment of RVV types to the element size.

Ping. If we all agree to use builtin types to model RVV types, is there any other issues we need to address in this patch?

I wonder if these types should be prefixed with "__clang_" like AArch64 tuple types?

clang/lib/AST/ASTContext.cpp
3875

Should this be indented 2 more spaces? to align with the uin64_t above?

3882

Same here

clang/lib/CodeGen/CGDebugInfo.cpp
783

Put this above the "if (FixedSize < 64)" and make that an "else if" so all assignments are in the same if/else structure.
You could pre-initialize Fractional to false and only assign it to true in the if/else chain.

clang/test/CodeGen/RISCV/riscv-v-debuginfo.c
2

This seems to be dependent on the patch that adds the vadd builtins?

HsiangKai updated this revision to Diff 319144.Jan 25 2021, 3:30 PM

Address @craig.topper's comments.

HsiangKai marked 4 inline comments as done.Jan 25 2021, 3:31 PM

I wonder if these types should be prefixed with "__clang_" like AArch64 tuple types?

It seems only AArch64 tuple types have "__clang_" prefix.

I wonder if these types should be prefixed with "__clang_" like AArch64 tuple types?

It seems only AArch64 tuple types have "__clang_" prefix.

I think the other types don't have it because they are defined by an ARM standard. See the description for https://reviews.llvm.org/D81721

I wonder if these types should be prefixed with "__clang_" like AArch64 tuple types?

It seems only AArch64 tuple types have "__clang_" prefix.

I think the other types don't have it because they are defined by an ARM standard. See the description for https://reviews.llvm.org/D81721

It seems to use __clang_ to differentiate tuple types and non-tuple types. The mangling rules are different between tuple types and non-tuple types in ARM. We have no mangling rules in our specification. I have a TODO item in this patch to clarify the mangling rules later.

This revision is now accepted and ready to land.Feb 17 2021, 10:24 AM
This revision was landed with ongoing or failed builds.Feb 17 2021, 6:18 PM
This revision was automatically updated to reflect the committed changes.