This is an archive of the discontinued LLVM Phabricator instance.

Add missing min/max builtin functions for some FP instructions to be generated from C-written codes
Needs RevisionPublic

Authored by joshua-arch1 on Feb 27 2023, 12:20 AM.

Details

Summary

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.

Diff Detail

Event Timeline

joshua-arch1 created this revision.Feb 27 2023, 12:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2023, 12:20 AM
joshua-arch1 requested review of this revision.Feb 27 2023, 12:20 AM
joshua-arch1 edited the summary of this revision. (Show Details)
joshua-arch1 edited the summary of this revision. (Show Details)

This is target independent code. The title shouldn’t refer to Zfa and will likely need none RISC-V reviewers.

joshua-arch1 retitled this revision from Add some missing builtin functions for zfa instructions to be generated from C-written codes to Add missing builtin functions for some FP instructions to be generated from C-written codes.Feb 27 2023, 12:41 AM
joshua-arch1 edited the summary of this revision. (Show Details)
craig.topper added inline comments.Feb 27 2023, 12:43 AM
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?

joshua-arch1 added a comment.EditedFeb 27 2023, 1:04 AM

This is target independent code. The title shouldn’t refer to Zfa and will likely need none RISC-V reviewers.

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?

craig.topper added inline comments.Feb 27 2023, 1:06 AM
clang/include/clang/Basic/Builtins.def
272

I think maybe C23 added fmaximum and fminimum as functions.

efriedma added inline comments.Feb 27 2023, 11:30 AM
clang/include/clang/Basic/Builtins.def
1366

C23 adds some new min/max functions, I think, but not under these names.

1448

Please split "roundeven" into a separate patch.

joshua-arch1 retitled this revision from Add missing builtin functions for some FP instructions to be generated from C-written codes to Add missing min/max builtin functions for some FP instructions to be generated from C-written codes.Feb 27 2023, 5:55 PM
joshua-arch1 edited the summary of this revision. (Show Details)
joshua-arch1 marked an inline comment as done.Feb 28 2023, 6:52 PM

Ping.

clang/include/clang/Basic/Builtins.def
272

Any reference?

1366

Could you please provide some reference?

craig.topper added inline comments.Feb 28 2023, 6:54 PM
clang/include/clang/Basic/Builtins.def
272
aaron.ballman added inline comments.Mar 2 2023, 7:48 AM
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.

efriedma added inline comments.Mar 6 2023, 9:12 AM
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.

This comment was removed by joshua-arch1.
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_num/fminimum_num/fmaximum_mag_num, and fminimum_mag_num functions only in their treatment of NaNs.

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.

aaron.ballman added inline comments.Mar 8 2023, 6:02 AM
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?

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 ?

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.

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 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.

I have already attached a standard version to LIBBUILTIN. So what we are discussing is whether we need to limit it to C23?

joshua-arch1 added a comment.EditedMar 10 2023, 1:41 AM

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 tried to use atomic_thread_fence() which is a new API in C11. It seems that there are no restrictions.

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.

I have already attached a standard version to LIBBUILTIN. So what we are discussing is whether we need to limit it to C23?

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.

Ping. Can this patch be landed now?

This revision is now accepted and ready to land.Mar 21 2023, 10:22 AM
craig.topper requested changes to this revision.EditedMar 21 2023, 7:39 PM

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.

This revision now requires changes to proceed.Mar 21 2023, 7:39 PM
joshua-arch1 added a comment.EditedMar 21 2023, 7:52 PM

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.

joshua-arch1 added a comment.EditedMar 21 2023, 7:54 PM

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.

I didn't see any regression failures in all the tests in llvm/test.

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.

I didn't see any regression failures in all the tests in llvm/test.

https://godbolt.org/z/76fnzf8hE

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.

I didn't see any regression failures in all the tests in llvm/test.

https://godbolt.org/z/76fnzf8hE

That is not caused by this patch. If we do not add -mattr=+experimental-zfa for llc, we cannot do instruction selection for fmaximum.

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.

I didn't see any regression failures in all the tests in llvm/test.

https://godbolt.org/z/76fnzf8hE

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.

joshua-arch1 added a comment.EditedMar 21 2023, 11:14 PM

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.

I didn't see any regression failures in all the tests in llvm/test.

https://godbolt.org/z/76fnzf8hE

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?

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.

I didn't see any regression failures in all the tests in llvm/test.

https://godbolt.org/z/76fnzf8hE

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.

joshua-arch1 added a comment.EditedMar 22 2023, 12:22 AM

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.

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.

Please do that as a new review. Drastically modifying a review is confusing.