[Discussion] Vector Intrinsic header file organization

[riscv-non-isa]

[Discussion] Vector Intrinsic header file organization

[kito-cheng]

We didn't have spec about the header file, only specify the type and interface, personally I prefer don't define that, and using pragma like SVE's implementation[1] in the implementation side for reduce the compilation time.

@hanna-kruppe did you have any preference on this issue?

[1] https://github.com/gcc-mirror/gcc/blob/master/gcc/config/aarch64/arm_sve.h#L40

[hanna-kruppe]

I'm not sure right now if this implementation strategy is also supported in Clang, but that's probably secondary for this question. As far as end users are concerned, I think the simplest and most consistent (with other architectures and their extensions) option is a single header file for all intrinsics related to the "V" extension. Further extensions that build on V but aren't typically implied by it (e.g., Zediv) could either be in a separate header per extension, or in the same header file (conditional on the extension being enabled by command line / target triple).

[zakk0610]

Hi all,
Intrinsic RFC API uses fixed-width type,
so I think the spec need to define rvv header should include stdint.h and at least define float32_t and float64_t,
any suggestion?

[rdolbeau]

@zakk0610 Agreed on stdint.h. However defining float32_t and float64_t will cause issue with other headers defining them for similar reasons, the name is too obvious. They are already in boost namespace, libdc1394, Arm's neon header... if you want to define some fixed-width floating-point types, they probably should use a somewhat namespaced identifier to avoid potential naming conflicts.

[zakk0610]

@rdolbeau good point, but I'm not sure... because SVE did the similar thing...
https://github.com/gcc-mirror/gcc/blob/master/gcc/config/aarch64/arm_sve.h#L32-L33

I guess ideally user should not include different target intrinsic header at the same time. (ex. include arm_sve.h and arm_neon.h)
BTW, don't forget arm neon also has the naming conflict issue when user use boost, maybe user program need to care it by itself?

[rdolbeau]

@zakk0610 I don't think we should follow Arm's mistake here - it's theirs to fix. Also IIRC POSIX reserves that suffix [after checking: yes they do, https://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html]. Of course as a system header this is still conceivable, but better safe than sorry...

[kito-cheng]

Just for reference, x86 intrinsic using __int32 / __int64 rather than int32_t / int64_t.

https://software.intel.com/sites/landingpage/IntrinsicsGuide/#text=bsr&expand=375

[PkmX]

I also think the header file should not be defining common type names like float32_t or float64_t to avoid breaking downstream projects.

For fixed-width integer types using intN_t or uintN_t from <stdint.h> should be okay, since they are already in the C standard. For floating-point types _Float16/_Float32/_Float64 as defined in ISO/IEC TS 18661-3:2015 and C2X can be used since they are guaranteed to be in IEEE binary16/binary32/binary64 format.

[kito-cheng]

There is some issue on _Float16/_Float32/_Float64, GCC didn't support those type in C++ mode, but clang support on both C/C++, I'll propose GCC to support those type in both C/C++ recently to GCC community.

[ebahapo]

Methinks that using the definitions in stdint.h is fine, since it's well established.

I also agree with @rdolbeau about defining the FP types internally in a separate name space, perhaps as x86 did with integers, as @kito-cheng pointed out: __float16_t, __float32_t, __float64_t.

[kito-cheng]

Just found another reason to define RISC-V own fixed-width floating point types, clang only support _Float16; _Float32 and _Float64 are unsupported...

[zakk0610]

I agree on @rdolbeau's point to support fixed-width floating point types with separate name space. Maybe __float16_t, __float32_t, __float64_t is a good idea.

[jrtc27]

FreeBSD defines __int32_t etc in its system headers (which is allowed, it's part of the C implementation). It doesn't currently define __float32_t etc, just __float_t/__double_t, though it might conceivably want to at some point. I'd suggest using a name that has a v or rvv in it to ensure no collisions with other separate parts of the C implementation out of your control.

[zakk0610]

On second thought, before we have consensus in naming, maybe we could use float and double directly if we want to avoid any collisions with other project?

[Hsiangkai]

FreeBSD defines __int32_t etc in its system headers (which is allowed, it's part of the C implementation). It doesn't currently define __float32_t etc, just __float_t/__double_t, though it might conceivably want to at some point. I'd suggest using a name that has a v or rvv in it to ensure no collisions with other separate parts of the C implementation out of your control.

We are going to upstream the implementation. From the above discussions, we all agree to define RISC-V specific fixed-width floating point types. I agree with @jrtc27 to have v or rvv in the type name to ensure no collisions. How about to define these fixed-width floating point types as rvfloat16_t, rvfloat32_t and rvfloat64_t?

[zakk0610]

LGTM to define rvfloat16_t, rvfloat32_t and rvfloat64_t.

[aditya4d1]

For vectors? @Hsiangkai

[Hsiangkai]

For vectors? @Hsiangkai

The proposal is for the general purpose 16/32/64 bits floating point types. rv is for risc-v. I know it is not appropriate to discuss it under the rvv-intrinsic-doc repository. If we all agree about it, we could give the proposal to https://github.com/riscv/riscv-c-api-doc/blob/master/riscv-c-api.md. It may be a better place to discuss the type names for RISC-V.

[jrtc27]

I'd suggest still using leading double underscores so you stay within the reserved-for-implementations namespace, otherwise you could in theory clash with a random application or library out there.

[Hsiangkai]

I'd suggest still using leading double underscores so you stay within the reserved-for-implementations namespace, otherwise you could in theory clash with a random application or library out there.

It is inconsistent with vector types, such as vint8m1_t. I agree to use double underscores to treat the types as reserved name to aoivd the conflict with users' code. If we decide to use double underscores prefix, we should modify the vector types in the same way. In addition, as @rdolbeau said, ending with _t is reserved by GNU C library. Should we consider avoid to end the types with _t?

[Hsiangkai]

Summarize the discussion points for fixed-size floating point types.

• Put RISC-V specific name into the type names to avoid name conflict.
• Avoid to use reserved name in other standard, e.g., _t is reserved by GNU C library.
• Use double underscores to avoid name conflict with application or library code.

We have some existing proposal here.

float16_t, float32_t, float64_t (ARM's implementation)
_Float16, _Float32, _Float64 (defined in ISO/IEC TS 18661-3:2015)
__float16_t, __float32_t, __float64_t
rvfloat16_t, rvfloat32_t, rvfloat64_t
__rvf16, __rvf32, __rvf64 (To use double underscores and remove _t in concise form.)

[pcwang-thead]

Hey guys, can we continue this discussion and reach a decision?

[aditya4d1]

Reading through the comments, I think there is enough support for __rvf16, __rvf32, __rvf64.
For vectors, I think we have to resume the discussion

[eopXD]

Maybe we can take a step back and don't define them in the header and let users use typedef to bridge the usages with the RVV intrinsics as currently LLVM / GCC provides _Float16 / float / double.

Any thoughts on this?

[eopXD]

With _Float16 supported in both compilers in C and C++, I think we don't need an extra typedef now.