To generate FMINM/FMAXM instructions in https://reviews.llvm.org/D143982, we need to use llvm intrinsics in IR files. Now I add some corresponding builtin functions to make sure these instructions can be generated from C codes.
Details
Diff Detail
Unit Tests
Event Timeline
This is target independent code. The title shouldn’t refer to Zfa and will likely need none RISC-V reviewers.
clang/include/clang/Basic/Builtins.def | ||
---|---|---|
272 | I can’t find these names in a Google search. Can you point me to documentation with these names? |
Can you introduce some none RISC-V reviews who may be familiar with this issue to me?
clang/include/clang/Basic/Builtins.def | ||
---|---|---|
272 | There are no corresponding functions in math. Maybe we can define riscv-dependent builtins. Any other suggestions on how to generate fmaxm/fminm zfa instructions from C codes? |
clang/include/clang/Basic/Builtins.def | ||
---|---|---|
272 | I think maybe C23 added fmaximum and fminimum as functions. |
clang/include/clang/Basic/Builtins.def | ||
---|---|---|
272 |
clang/include/clang/Basic/Builtins.def | ||
---|---|---|
272 | Should we be adding fmaximum_num and fminimum_num at the same time, given that these are effectively the same functions (differ only in how they handle NaNs)? And is there a plan for the *_mag and *_mag_num variants? (Not trying to scope creep, just curious what the plans are.) |
Ping.
clang/include/clang/Basic/Builtins.def | ||
---|---|---|
272 | According to C23 standard, I didn't see any difference between famximum and fmaximum_num. As for fmaximum_mag, there are no corresponding intrinsics, so I didn't add it in this patch. |
clang/include/clang/Basic/Builtins.def | ||
---|---|---|
272 | Are you looking in the right spot? Appendix F goes into more detail (F.10.9.5) . The structure of the standard is a little weird because it allows non-IEEE floats. |
clang/include/clang/Basic/Builtins.def | ||
---|---|---|
272 | Sorry I confused *_num with *_mag. According to F.10.9.5, the fmaximum/fminimum/fmaximum_mag/fminimum_mag functions differ from the corresponding fmaximum/fminimum in C23 match maximum/minimum in IEEE Std 754-2019. It returns a quiet NaN if either operand is a NaN. I cannot find any differences between fmaximum/fminimum and fmaximum_mag/fminimum_mag. fmaximum_num/fminimum_num in C23 match maximumNumber/minimumNumber in IEEE Std 754-2019. It returns a number if one operand is a number and the other is a NaN. There are no corresponding intrinsics yet. We can implement it later. |
clang/include/clang/Basic/Builtins.def | ||
---|---|---|
272 | Thanks for looking into it, I'm fine implementing that work later. |
Do we need to limit this to C23 being enabled or just rely on the linker error if there is no hardware support and an older libm i used?
@efriedma What's your idea since there will be similar issues for my last patch https://reviews.llvm.org/D144935 ?
FWIW, I think this somewhat relates to recent discussions we've been having about freestanding: https://reviews.llvm.org/D144889#4157257 where there seems to be a sentiment of pushing this onto the user/linker. But it might be nice for us to document a policy for this sort of thing, especially given that C23 added a lot of new math APIs.
I don't really care what happens if someone uses a __builtin_ prefixed name that the underlying libc doesn't support; user should know the semantics of builtins they use. (Some __builtin names refer to functions the compiler can't realistically provide even if we wanted to.)
It might make sense to attach a standard version to LIBBUILTIN, I guess. Not because we care about link errors, but so that we don't break someone's pre-C23 code that uses the name "fminimum" to mean something else. We haven't really paid attention to this in the past... it didn't really matter much for a while since we didn't really get many new APIs since C99.
Heh, C11 brought a fair number of new APIs as well (atomics and threads) but they were optional features and you're right about us not really paying attention to that sort of thing. But we've put efforts into warning about reserved identifiers recently and this idea would dovetail nicely with that work.
I have already attached a standard version to LIBBUILTIN. So what we are discussing is whether we need to limit it to C23?
I tried to use atomic_thread_fence() which is a new API in C11. It seems that there are no restrictions.
LIBBUILTIN can specify what language *mode* the builtin is available in, but it doesn't allow you to specify what versions of those language modes are allowed. We're trying to decide whether we need such functionality.
I tried to use atomic_thread_fence() which is a new API in C11. It seems that there are no restrictions.
Yeah, we've not done the implementation work to handle checking for reserved builtin IDs yet.
I think we do need such functionality, but not as a prerequisite for landing the current patch. We already steal names from the user with other builtins, so this only adds a small amount of additional risk for collision. But C2x adds hundreds of new APIs for floating-point operations (decimal floating point and binary floating point), so this probably needs to be resolved sooner rather than later.
The llvm.maximum/minimum intrinsics crash the backend on at least RISC-V and X86 targets right now rather than falling back to a libcall. I don't think we can approve this patch without a stable backend for all targets.
What caused this crash? I think adding target-independent builtins will not influence the backend.
That is not caused by this patch. If we do not add -mattr=+experimental-zfa for llc, we cannot do instruction selection for fmaximum.
Right. But this patch doesn't check for RISC-V or Zfa being enabled so any use of __builtin_fmaximum or maximum will crash the compiler if used on a non RISC-V target or on RISC-V without Zfa.
But you know builtins are hard to check for RISC-V or Zfa in clang. If we do not use builtin_fmaximum/builtin_fminimum, how can we generate fmaxm/fminm from C-code in clang?
Regardless of this patch I think we need to add builtins for fmaxm/fminm to BuiltinsRISCV.def. The C fmaximum/fminimum functions are only defined in the math.h header file for C23. We need builtins and intrinsics for RISC-V that are independent of that. RISC-V International is supposed to be forming a task group for defining all RISC-V scalar intrinsics.
But you know builtins are hard to check for RISC-V or Zfa in clang. If we do not use builtin_fmaximum/builtin_fminimum, how can we generate fmaxm/fminm from C-code in clang?
Regardless of this patch I think we need to add builtins for fmaxm/fminm to BuiltinsRISCV.def. The C fmaximum/fminimum functions are only defined in the math.h header file for C23. We need builtins and intrinsics for RISC-V that are independent of that. RISC-V International is supposed to be forming a task group for defining all RISC-V scalar intrinsics.
OK, I'll modify this change by adding builtins for fmaxm/fminm to BuiltinsRISCV.def.
I can’t find these names in a Google search. Can you point me to documentation with these names?