Mark libm builtins as such.

[• chatur01]

Summary

The builtins didn't have the F flag set on them. D9912 relies on this change.

[rengolin]

Isn't this the case for the others below, too?

[• chatur01]

I missed signbit, the other maths related builtins do not appear to be standard libm builtins, so I've left them out.

Thanks very much for the review :)

[• chatur01]

Make the signbit parameter generic to handle real-floating type.

[• chatur01]

Ping.

[rengolin]

Hi Charlie,

I think this looks ok, but can you enlighten me a bit?

From what I read here [1], these builtins can be compiler replacements for library functions, especially if they have the "__builtin" prefix:

"... may be handled as built-in functions. All these functions have corresponding versions prefixed with __builtin_ ..."

And from the bug report [2], what Android needs is for it to be re-declared differently, and that's why you're adding them as library calls, so that can happen.

Is then, this patch ultimately linked to D9912? Or does this make sense on its own? If it does, I'm failing to see why. :)

cheers,
--renato

[1] https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html
[2] https://llvm.org/bugs/show_bug.cgi?id=20958

[• chatur01]

Hey Renato, thanks very much for taking another look.

In D9913#192654, @rengolin wrote:

From what I read here [1], these builtins can be compiler replacements for library functions, especially if they have the "__builtin" prefix:

Right. There are still some omissions in what Clang marks up a library builtin and what GCC does outside strict ISO C mode.

You already pointed out the signbit omissions, but here are all the ones I still haven't marked up after this patch

BUILTIN(__builtin_signbitf, "if", "nc")
BUILTIN(__builtin_signbitl, "iLd", "nc")
BUILTIN(__builtin_ffsll, "iLLi", "nc")
BUILTIN(__builtin_bcmp, "iv*v*z", "n")
BUILTIN(__builtin_alloca, "v*z"   , "n")

I wasn't sure if Clang would be interested in compatibility in this case. So I originally focused on the C99 floating point stuff that is part of a standard I could read. Do you think I should patch up the above missing bits too?

"... may be handled as built-in functions. All these functions have corresponding versions prefixed with __builtin_ ..."

And from the bug report [2], what Android needs is for it to be re-declared differently, and that's why you're adding them as library calls, so that can happen.

That's basically it. These C99 floating-point functions are defined as taking either a float, a double or a long double as argument(s). Operationally, we don't want to commit to declaring one of these options too soon, since the user (as in the Android case) might want to redeclare it in an incompatible (but legal) way later. What Clang currently does is declare the type-generic builtins like this with an empty parameter list, which gives weird behaviour in both C and C++ when you try to redeclare them. I'm marking them up as library calls so that I can selectively handle just the builtins I need to in my patch in D9912.

Is then, this patch ultimately linked to D9912? Or does this make sense on its own? If it does, I'm failing to see why. :)

I tried to mention that with my link in Phabricator, but I realize I've failed to make it clear. There doesn't seem to be a good way of posting a logical patch set to Phab. Aside from D9912 relying on this change, I also think it's fine to go in on its own, since these builtins should be marked up as "F", given that they're listed in the C99 standard. Do you agree?

Thanks again for your time,

• Charlie.

[1] https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html
[2] https://llvm.org/bugs/show_bug.cgi?id=20958

[rengolin]

In D9913#192741, @chatur01 wrote:

I wasn't sure if Clang would be interested in compatibility in this case. So I originally focused on the C99 floating point stuff that is part of a standard I could read. Do you think I should patch up the above missing bits too?

That's my point. If this patch makes sense on its own, we should mark them all. I have no idea how to test it, though.

What Clang currently does is declare the type-generic builtins like this with an empty parameter list, which gives weird behaviour in both C and C++ when you try to redeclare them. I'm marking them up as library calls so that I can selectively handle just the builtins I need to in my patch in D9912.

There are many odd things about this... First, Clang declaring it empty looks like a hack in Clang to "cross that bridge when we get there" kind of "solution". Second, marking them as library calls, when they're really builtin versions of library calls, is also a bit dodgy. Finally, Android has a bad reputation of re-implementing low-level libraries by calling higher-level libraries (for example, __aeabi_memcpy calling libc's memcpy), which is *also* shoddy.

I have no preference, but it would be good to be only as consistent as we need to be. Ie. not enable things we're not going to use just because it's similar to the ones we will use.

Since this smells like another hack to make Android work without really fixing Clang, I'd suggest moving it to the original patch (D9912) with a good comment about the reason we're doing this.

I tried to mention that with my link in Phabricator, but I realize I've failed to make it clear. There doesn't seem to be a good way of posting a logical patch set to Phab.

There isn't. That's why I do a (git diff -U999 master > patch) every time and deal with patch-sets only locally. Gerrit seems to be a lot better on this, but once you have submitted a patch set, squashing and rebasing gets *a lot* worse, so I'm not fond of either way. Github is better.

Aside from D9912 relying on this change, I also think it's fine to go in on its own, since these builtins should be marked up as "F", given that they're listed in the C99 standard. Do you agree?

AFAICS, they're not listed in C99, only their counterparts without the __builtin_ are. IIUC, these builtins are the compiler trying to be smart about implementation details, and that's why Android shouldn't be fiddling with them in the first place. But alas, that's the way of the Android. Whatever compiles, ship it.

My opinion is to move this into D9912 and only change the ones you really need, with a comment on why you're changing (maybe a FIXME?).

cheers,
--renato

[hfinkel]

In D9913#192821, @rengolin wrote:

In D9913#192741, @chatur01 wrote:

I wasn't sure if Clang would be interested in compatibility in this case. So I originally focused on the C99 floating point stuff that is part of a standard I could read. Do you think I should patch up the above missing bits too?

That's my point. If this patch makes sense on its own, we should mark them all.

I agree.

I have no idea how to test it, though.

What Clang currently does is declare the type-generic builtins like this with an empty parameter list, which gives weird behaviour in both C and C++ when you try to redeclare them. I'm marking them up as library calls so that I can selectively handle just the builtins I need to in my patch in D9912.

There are many odd things about this... First, Clang declaring it empty looks like a hack in Clang to "cross that bridge when we get there" kind of "solution". Second, marking them as library calls, when they're really builtin versions of library calls, is also a bit dodgy. Finally, Android has a bad reputation of re-implementing low-level libraries by calling higher-level libraries (for example, __aeabi_memcpy calling libc's memcpy), which is *also* shoddy.

I have no preference, but it would be good to be only as consistent as we need to be. Ie. not enable things we're not going to use just because it's similar to the ones we will use.

Since this smells like another hack to make Android work without really fixing Clang, I'd suggest moving it to the original patch (D9912) with a good comment about the reason we're doing this.

I tried to mention that with my link in Phabricator, but I realize I've failed to make it clear. There doesn't seem to be a good way of posting a logical patch set to Phab.

There isn't. That's why I do a (git diff -U999 master > patch) every time and deal with patch-sets only locally. Gerrit seems to be a lot better on this, but once you have submitted a patch set, squashing and rebasing gets *a lot* worse, so I'm not fond of either way. Github is better.

Aside from D9912 relying on this change, I also think it's fine to go in on its own, since these builtins should be marked up as "F", given that they're listed in the C99 standard. Do you agree?

AFAICS, they're not listed in C99, only their counterparts without the __builtin_ are. IIUC, these builtins are the compiler trying to be smart about implementation details

I don't understand this part of the conversation. include/clang/Basic/Builtins.def says:

//  F -> this is a libc/libm function with a '__builtin_' prefix added.
//  f -> this is a libc/libm function without the '__builtin_' prefix. It can
//       be followed by ':headername:' to state which header this function
//       comes from.

this patch does not mark these with 'f', but with 'F', and they are indeed libm functions with __builtin_ added.

, and that's why Android shouldn't be fiddling with them in the first place. But alas, that's the way of the Android. Whatever compiles, ship it.

My opinion is to move this into D9912 and only change the ones you really need, with a comment on why you're changing (maybe a FIXME?).

cheers,
--renato

[rengolin]

In D9913#202491, @hfinkel wrote:

That's my point. If this patch makes sense on its own, we should mark them all.

I agree.

Ok, let's just do it then. I believe this is clearly a typo/forgotten kind of thing.

Charlie, can you mark all of the affected builtins with "F"?

I don't understand this part of the conversation. include/clang/Basic/Builtins.def says:

//  F -> this is a libc/libm function with a '__builtin_' prefix added.
//  f -> this is a libc/libm function without the '__builtin_' prefix. It can
//       be followed by ':headername:' to state which header this function
//       comes from.

this patch does not mark these with 'f', but with 'F', and they are indeed libm functions with __builtin_ added.

That's the point. "f" is for libc functions that don't get overriden by the compiler with a "__builtin_" prefix. All of these *do* have prefix, so they need to be marked as "F".

The point here being that __builtin_isnan, for instance, is a builtin that can be either lowered as a sequence of instructions, or as a libc call to isnan, depending on the target. This is precisely the meaning of "F".

cheers,
--renato

[• chatur01]

Thanks very much Renato and Hal for giving this one another look and apologies for my delay getting back, I've been on holiday.

I have updated the patch to mark up all libm functions as described in the first section of,

https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html

Which are the functions GCC has defined as being part of libc implementations. The other builtins listed on that page are there just for optimization purposes.

The way I tested this was to take each chunk of functions listed in each of the ISO C* paragraphs from the above page, and grep for each function name in Builtins.def, checking whether F was part of that builtin's mark up. After this patch, all those checks pass.

OK to land?

[• chatur01]

Thanks Renato, r242675 landed.

include/clang/Basic/Builtins.def

 BUILTIN(__builtin_ctanhl, "XLdXLd", "Fnc")
 
 // FP Comparisons.
-BUILTIN(__builtin_isgreater     , "i.", "nc")
-BUILTIN(__builtin_isgreaterequal, "i.", "nc")
-BUILTIN(__builtin_isless        , "i.", "nc")
-BUILTIN(__builtin_islessequal   , "i.", "nc")
-BUILTIN(__builtin_islessgreater , "i.", "nc")
-BUILTIN(__builtin_isunordered   , "i.", "nc")
+BUILTIN(__builtin_isgreater     , "i.", "Fnc")
+BUILTIN(__builtin_isgreaterequal, "i.", "Fnc")
+BUILTIN(__builtin_isless        , "i.", "Fnc")
+BUILTIN(__builtin_islessequal   , "i.", "Fnc")
+BUILTIN(__builtin_islessgreater , "i.", "Fnc")
+BUILTIN(__builtin_isunordered   , "i.", "Fnc")
 
 // Unary FP classification
-BUILTIN(__builtin_fpclassify, "iiiii.", "nc")
-BUILTIN(__builtin_isfinite,   "i.", "nc")
-BUILTIN(__builtin_isinf,      "i.", "nc")
-BUILTIN(__builtin_isinf_sign, "i.", "nc")
-BUILTIN(__builtin_isnan,      "i.", "nc")
-BUILTIN(__builtin_isnormal,   "i.", "nc")
+BUILTIN(__builtin_fpclassify, "iiiii.", "Fnc")
+BUILTIN(__builtin_isfinite,   "i.", "Fnc")
+BUILTIN(__builtin_isinf,      "i.", "Fnc")
+BUILTIN(__builtin_isinf_sign, "i.", "Fnc")
+BUILTIN(__builtin_isnan,      "i.", "Fnc")
+BUILTIN(__builtin_isnormal,   "i.", "Fnc")
 
 // FP signbit builtins
-BUILTIN(__builtin_signbit, "id", "nc")
-BUILTIN(__builtin_signbitf, "if", "nc")
-BUILTIN(__builtin_signbitl, "iLd", "nc")
+BUILTIN(__builtin_signbit, "i.", "Fnc")
+BUILTIN(__builtin_signbitf, "if", "Fnc")
+BUILTIN(__builtin_signbitl, "iLd", "Fnc")
 
 // Builtins for arithmetic.
 BUILTIN(__builtin_clzs , "iUs"  , "nc")

 BUILTIN(__builtin_ctzl , "iULi" , "nc")
 BUILTIN(__builtin_ctzll, "iULLi", "nc")
 // TODO: int ctzimax(uintmax_t)
-BUILTIN(__builtin_ffs  , "ii"  , "nc")
-BUILTIN(__builtin_ffsl , "iLi" , "nc")
-BUILTIN(__builtin_ffsll, "iLLi", "nc")
+BUILTIN(__builtin_ffs  , "ii"  , "Fnc")
+BUILTIN(__builtin_ffsl , "iLi" , "Fnc")
+BUILTIN(__builtin_ffsll, "iLLi", "Fnc")
 BUILTIN(__builtin_parity  , "iUi"  , "nc")
 BUILTIN(__builtin_parityl , "iULi" , "nc")
 BUILTIN(__builtin_parityll, "iULLi", "nc")

 BUILTIN(__builtin_va_copy, "vAA", "n")
 BUILTIN(__builtin_stdarg_start, "vA.", "n")
 BUILTIN(__builtin_assume_aligned, "v*vC*z.", "nc")
-BUILTIN(__builtin_bcmp, "iv*v*z", "n")
+BUILTIN(__builtin_bcmp, "iv*v*z", "Fn")
 BUILTIN(__builtin_bcopy, "vv*v*z", "n")
 BUILTIN(__builtin_bzero, "vv*z", "nF")
 BUILTIN(__builtin_fprintf, "iP*cC*.", "Fp:1:")

 BUILTIN(__builtin_unreachable, "v", "nr")
 BUILTIN(__builtin_shufflevector, "v."   , "nc")
 BUILTIN(__builtin_convertvector, "v."   , "nct")
-BUILTIN(__builtin_alloca, "v*z"   , "n")
+BUILTIN(__builtin_alloca, "v*z"   , "Fn")
 BUILTIN(__builtin_call_with_static_chain, "v.", "nt")
 
 // "Overloaded" Atomic operator builtins.  These are overloaded to support data