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

[joshua-arch1]

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.

[craig.topper]

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

[craig.topper]

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]

In D144843#4154375, @craig.topper wrote:

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]

clang/include/clang/Basic/Builtins.def

272

I think maybe C23 added fmaximum and fminimum as functions.

[efriedma]

clang/include/clang/Basic/Builtins.def

1361

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

1443

Please split "roundeven" into a separate patch.

[joshua-arch1]

Ping.

clang/include/clang/Basic/Builtins.def

272

Any reference?

1361

Could you please provide some reference?

[craig.topper]

clang/include/clang/Basic/Builtins.def

272

https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3054.pdf 7.12.12.4

[joshua-arch1]

Ping.

[aaron.ballman]

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

[joshua-arch1]

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]

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.

[joshua-arch1]

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]

clang/include/clang/Basic/Builtins.def

272

Thanks for looking into it, I'm fine implementing that work later.

[craig.topper]

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?

[joshua-arch1]

In D144843#4178746, @craig.topper wrote:

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 ?

[aaron.ballman]

In D144843#4179908, @joshua-arch1 wrote:

In D144843#4178746, @craig.topper wrote:

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.

[efriedma]

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.

[aaron.ballman]

In D144843#4182175, @efriedma wrote:

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.

[joshua-arch1]

In D144843#4182175, @efriedma wrote:

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]

In D144843#4182262, @aaron.ballman wrote:

In D144843#4182175, @efriedma wrote:

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.

[aaron.ballman]

In D144843#4183838, @joshua-arch1 wrote:

In D144843#4182175, @efriedma wrote:

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.

[joshua-arch1]

Ping. Can this patch be landed now?

[efriedma]

LGTM

[craig.topper]

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.

[joshua-arch1]

In D144843#4211898, @craig.topper wrote:

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]

In D144843#4211898, @craig.topper wrote:

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.

[craig.topper]

In D144843#4211910, @joshua-arch1 wrote:

In D144843#4211898, @craig.topper wrote:

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

[joshua-arch1]

In D144843#4211967, @craig.topper wrote:

In D144843#4211910, @joshua-arch1 wrote:

In D144843#4211898, @craig.topper wrote:

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.

[craig.topper]

In D144843#4212182, @joshua-arch1 wrote:

In D144843#4211967, @craig.topper wrote:

In D144843#4211910, @joshua-arch1 wrote:

In D144843#4211898, @craig.topper wrote:

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]

In D144843#4212198, @craig.topper wrote:

In D144843#4212182, @joshua-arch1 wrote:

In D144843#4211967, @craig.topper wrote:

In D144843#4211910, @joshua-arch1 wrote:

In D144843#4211898, @craig.topper wrote:

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?

[craig.topper]

In D144843#4212205, @joshua-arch1 wrote:

In D144843#4212198, @craig.topper wrote:

In D144843#4212182, @joshua-arch1 wrote:

In D144843#4211967, @craig.topper wrote:

In D144843#4211910, @joshua-arch1 wrote:

In D144843#4211898, @craig.topper wrote:

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]

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.

[craig.topper]

In D144843#4212288, @joshua-arch1 wrote:

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.

clang/include/clang/Basic/Builtins.def

 BUILTIN(__builtin_fmaxf16, "hhh", "FncE")
 BUILTIN(__builtin_fmaxl, "LdLdLd", "FncE")
 BUILTIN(__builtin_fmaxf128, "LLdLLdLLd", "FncE")
 BUILTIN(__builtin_fmin, "ddd", "FncE")
 BUILTIN(__builtin_fminf, "fff", "FncE")
 BUILTIN(__builtin_fminf16, "hhh", "FncE")
 BUILTIN(__builtin_fminl, "LdLdLd", "FncE")
 BUILTIN(__builtin_fminf128, "LLdLLdLLd", "FncE")
+BUILTIN(__builtin_fmaximum, "ddd", "Fnc")
+BUILTIN(__builtin_fmaximumf, "fff", "Fnc")
+BUILTIN(__builtin_fmaximumf16, "hhh", "Fnc")
+BUILTIN(__builtin_fmaximuml, "LdLdLd", "Fnc")
+BUILTIN(__builtin_fmaximumf128, "LLdLLdLLd", "Fnc")
+BUILTIN(__builtin_fminimum, "ddd", "Fnc")
+BUILTIN(__builtin_fminimumf, "fff", "Fnc")
+BUILTIN(__builtin_fminimumf16, "hhh", "Fnc")
+BUILTIN(__builtin_fminimuml, "LdLdLd", "Fnc")
+BUILTIN(__builtin_fminimumf128, "LLdLLdLLd", "Fnc")
 BUILTIN(__builtin_hypot , "ddd"  , "Fne")
 BUILTIN(__builtin_hypotf, "fff"  , "Fne")
 BUILTIN(__builtin_hypotl, "LdLdLd", "Fne")
 BUILTIN(__builtin_hypotf128, "LLdLLdLLd", "Fne")
 BUILTIN(__builtin_ilogb , "id", "Fne")
 BUILTIN(__builtin_ilogbf, "if", "Fne")
 BUILTIN(__builtin_ilogbl, "iLd", "Fne")
 BUILTIN(__builtin_ilogbf128, "iLLd", "Fne")

 LIBBUILTIN(fmax, "ddd", "fnc", MATH_H, ALL_LANGUAGES)
 LIBBUILTIN(fmaxf, "fff", "fnc", MATH_H, ALL_LANGUAGES)
 LIBBUILTIN(fmaxl, "LdLdLd", "fnc", MATH_H, ALL_LANGUAGES)
 
 LIBBUILTIN(fmin, "ddd", "fnc", MATH_H, ALL_LANGUAGES)
 LIBBUILTIN(fminf, "fff", "fnc", MATH_H, ALL_LANGUAGES)
 LIBBUILTIN(fminl, "LdLdLd", "fnc", MATH_H, ALL_LANGUAGES)
 
+LIBBUILTIN(fmaximum, "ddd", "fnc", MATH_H, ALL_LANGUAGES)
+LIBBUILTIN(fmaximumf, "fff", "fnc", MATH_H, ALL_LANGUAGES)
+LIBBUILTIN(fmaximuml, "LdLdLd", "fnc", MATH_H, ALL_LANGUAGES)
+
+LIBBUILTIN(fminimum, "ddd", "fnc", MATH_H, ALL_LANGUAGES)
+LIBBUILTIN(fminimumf, "fff", "fnc", MATH_H, ALL_LANGUAGES)
+LIBBUILTIN(fminimuml, "LdLdLd", "fnc", MATH_H, ALL_LANGUAGES)
+
 LIBBUILTIN(hypot, "ddd", "fne", MATH_H, ALL_LANGUAGES)
 LIBBUILTIN(hypotf, "fff", "fne", MATH_H, ALL_LANGUAGES)
 LIBBUILTIN(hypotl, "LdLdLd", "fne", MATH_H, ALL_LANGUAGES)
 
 LIBBUILTIN(ilogb, "id", "fne", MATH_H, ALL_LANGUAGES)
 LIBBUILTIN(ilogbf, "if", "fne", MATH_H, ALL_LANGUAGES)
 LIBBUILTIN(ilogbl, "iLd", "fne", MATH_H, ALL_LANGUAGES)
 

 LIBBUILTIN(rintl, "LdLd", "fng", MATH_H, ALL_LANGUAGES)
 
 LIBBUILTIN(round, "dd", "fnc", MATH_H, ALL_LANGUAGES)
 LIBBUILTIN(roundf, "ff", "fnc", MATH_H, ALL_LANGUAGES)
 LIBBUILTIN(roundl, "LdLd", "fnc", MATH_H, ALL_LANGUAGES)
 
 LIBBUILTIN(scalbln, "ddLi", "fne", MATH_H, ALL_LANGUAGES)
 LIBBUILTIN(scalblnf, "ffLi", "fne", MATH_H, ALL_LANGUAGES)
 LIBBUILTIN(scalblnl, "LdLdLi", "fne", MATH_H, ALL_LANGUAGES)
 
 LIBBUILTIN(scalbn, "ddi", "fne", MATH_H, ALL_LANGUAGES)
 LIBBUILTIN(scalbnf, "ffi", "fne", MATH_H, ALL_LANGUAGES)
 LIBBUILTIN(scalbnl, "LdLdi", "fne", MATH_H, ALL_LANGUAGES)
 
 LIBBUILTIN(sin, "dd", "fne", MATH_H, ALL_LANGUAGES)
 LIBBUILTIN(sinf, "ff", "fne", MATH_H, ALL_LANGUAGES)
 LIBBUILTIN(sinl, "LdLd", "fne", MATH_H, ALL_LANGUAGES)

clang/lib/CodeGen/CGBuiltin.cpp

if (FD->hasAttr<ConstAttr>() ||
     case Builtin::BI__builtin_fminf:
     case Builtin::BI__builtin_fminf16:
     case Builtin::BI__builtin_fminl:
     case Builtin::BI__builtin_fminf128:
       return RValue::get(emitBinaryMaybeConstrainedFPBuiltin(*this, E,
                                    Intrinsic::minnum,
                                    Intrinsic::experimental_constrained_minnum));
 
+    case Builtin::BIfmaximum:
+    case Builtin::BIfmaximumf:
+    case Builtin::BIfmaximuml:
+    case Builtin::BI__builtin_fmaximum:
+    case Builtin::BI__builtin_fmaximumf:
+    case Builtin::BI__builtin_fmaximumf16:
+    case Builtin::BI__builtin_fmaximuml:
+    case Builtin::BI__builtin_fmaximumf128:
+      return RValue::get(emitBinaryMaybeConstrainedFPBuiltin(*this, E,
+                                   Intrinsic::maximum,
+                                   Intrinsic::experimental_constrained_maximum));
+
+    case Builtin::BIfminimum:
+    case Builtin::BIfminimumf:
+    case Builtin::BIfminimuml:
+    case Builtin::BI__builtin_fminimum:
+    case Builtin::BI__builtin_fminimumf:
+    case Builtin::BI__builtin_fminimumf16:
+    case Builtin::BI__builtin_fminimuml:
+    case Builtin::BI__builtin_fminimumf128:
+      return RValue::get(emitBinaryMaybeConstrainedFPBuiltin(*this, E,
+                                   Intrinsic::minimum,
+                                   Intrinsic::experimental_constrained_minimum));  
+
     // fmod() is a special-case. It maps to the frem instruction rather than an
     // LLVM intrinsic.
     case Builtin::BIfmod:
     case Builtin::BIfmodf:
     case Builtin::BIfmodl:
     case Builtin::BI__builtin_fmod:
     case Builtin::BI__builtin_fmodf:
     case Builtin::BI__builtin_fmodf16:

clang/test/CodeGen/builtins.c

void test_float_builtin_ops(float F, double D, long double LD) {
   // CHECK: call float @llvm.maxnum.f32
 
   resd = __builtin_fmax(D, D);
   // CHECK: call double @llvm.maxnum.f64
 
   resld = __builtin_fmaxl(LD, LD);
   // CHECK: call x86_fp80 @llvm.maxnum.f80
 
+  resf = __builtin_fminimumf(F, F);
+  // CHECK: call float @llvm.minimum.f32
+
+  resd = __builtin_fminimum(D, D);
+  // CHECK: call double @llvm.minimum.f64
+
+  resld = __builtin_fminimuml(LD, LD);
+  // CHECK: call x86_fp80 @llvm.minimum.f80
+
+  resf = __builtin_fmaximumf(F, F);
+  // CHECK: call float @llvm.maximum.f32
+
+  resd = __builtin_fmaximum(D, D);
+  // CHECK: call double @llvm.maximum.f64
+
+  resld = __builtin_fmaximuml(LD, LD);
+  // CHECK: call x86_fp80 @llvm.maximum.f80
+
   resf = __builtin_fabsf(F);
   // CHECK: call float @llvm.fabs.f32
 
   resd = __builtin_fabs(D);
   // CHECK: call double @llvm.fabs.f64
 
   resld = __builtin_fabsl(LD);
   // CHECK: call x86_fp80 @llvm.fabs.f80