Skip to content

[Clang] Fixes of builtin definitions after PR #68324. #81022

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

michele-scandale
Copy link
Contributor

This commit addresses few differences between the Builtins.def file before the refactoring done in PR #68324 and the currently generated Builtins.inc file.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Feb 7, 2024
@llvmbot
Copy link
Member

llvmbot commented Feb 7, 2024

@llvm/pr-subscribers-clang

Author: None (michele-scandale)

Changes

This commit addresses few differences between the Builtins.def file before the refactoring done in PR #68324 and the currently generated Builtins.inc file.


Full diff: https://github.com/llvm/llvm-project/pull/81022.diff

1 Files Affected:

  • (modified) clang/include/clang/Basic/Builtins.td (+43-27)
diff --git a/clang/include/clang/Basic/Builtins.td b/clang/include/clang/Basic/Builtins.td
index 31a2bdeb2d3e5e..a228334e069e5b 100644
--- a/clang/include/clang/Basic/Builtins.td
+++ b/clang/include/clang/Basic/Builtins.td
@@ -12,13 +12,17 @@ class FPMathTemplate : Template<["float", "double", "long double"],
                                 ["f",     "",       "l"]>;
 
 class FPMathWithF16Template :
-    Template<["float", "double", "long double", "__fp16", "__float128"],
-             ["f",     "",       "l",           "f16",    "f128"]>;
+    Template<["float", "double", "long double", "__fp16"],
+             ["f",     "",       "l",           "f16"]>;
 
 class FPMathWithF16F128Template :
     Template<["float", "double", "long double", "__fp16", "__float128"],
              ["f",     "",       "l",           "f16",    "f128"]>;
 
+class FPMathWithF128Template :
+    Template<["float", "double", "long double", "__float128"],
+             ["f",     "",       "l",           "f128"]>;
+
 class F16F128MathTemplate : Template<["__fp16", "__float128"],
                                      ["f16",    "f128"]>;
 
@@ -253,18 +257,30 @@ def FrexpF16F128 : F16F128MathTemplate, Builtin {
   let Prototype = "T(T, int*)";
 }
 
-def HugeVal : Builtin, FPMathWithF16F128Template {
+def HugeVal : Builtin, FPMathWithF128Template {
   let Spellings = ["__builtin_huge_val"];
   let Attributes = [NoThrow, Const, Constexpr];
   let Prototype = "T()";
 }
 
-def Inf : Builtin, FPMathWithF16F128Template {
+def HugeValF16 : Builtin {
+       let Spellings = ["__builtin_huge_valf16"];
+  let Attributes = [NoThrow, Const, Constexpr];
+  let Prototype = "_Float16()";
+}
+
+def Inf : Builtin, FPMathWithF128Template {
   let Spellings = ["__builtin_inf"];
   let Attributes = [NoThrow, Const, Constexpr];
   let Prototype = "T()";
 }
 
+def InfF16 : Builtin {
+  let Spellings = ["__builtin_inff16"];
+  let Attributes = [NoThrow, Const, Constexpr];
+  let Prototype = "_Float16()";
+}
+
 def LdexpF16F128 : F16F128MathTemplate, Builtin {
   let Spellings = ["__builtin_ldexp"];
   let Attributes = [FunctionWithBuiltinPrefix, NoThrow, ConstIgnoringErrnoAndExceptions];
@@ -1538,7 +1554,7 @@ def SyncBoolCompareAndSwap : Builtin {
 def SyncBoolCompareAndSwapN : Builtin, SyncBuiltinsTemplate {
   let Spellings = ["__sync_bool_compare_and_swap_"];
   let Attributes = [CustomTypeChecking, NoThrow];
-  let Prototype = "T(T volatile*, T, ...)";
+  let Prototype = "bool(T volatile*, T, T, ...)";
 }
 
 def SyncValCompareAndSwap : Builtin {
@@ -1550,7 +1566,7 @@ def SyncValCompareAndSwap : Builtin {
 def SynLockValCompareAndSwapN : Builtin, SyncBuiltinsTemplate {
   let Spellings = ["__sync_val_compare_and_swap_"];
   let Attributes = [CustomTypeChecking, NoThrow];
-  let Prototype = "T(T volatile*, T, ...)";
+  let Prototype = "T(T volatile*, T, T, ...)";
 }
 
 def SyncLockTestAndSet : Builtin {
@@ -1565,16 +1581,16 @@ def SynLockLockTestAndSetN : Builtin, SyncBuiltinsTemplate {
   let Prototype = "T(T volatile*, T, ...)";
 }
 
-def SyncLockReleaseN : Builtin {
+def SyncLockRelease : Builtin {
   let Spellings = ["__sync_lock_release"];
   let Attributes = [CustomTypeChecking];
   let Prototype = "void(...)";
 }
 
-def SynLockReleaseN : Builtin, SyncBuiltinsTemplate {
+def SyncLockReleaseN : Builtin, SyncBuiltinsTemplate {
   let Spellings = ["__sync_lock_release_"];
   let Attributes = [CustomTypeChecking, NoThrow];
-  let Prototype = "T(T volatile*, T, ...)";
+  let Prototype = "void(T volatile*, ...)";
 }
 
 def SyncSwap : Builtin {
@@ -2557,6 +2573,13 @@ def Abort : LibBuiltin<"stdlib.h"> {
   let AddBuiltinPrefixedAlias = 1;
 }
 
+def Abs : IntMathTemplate, LibBuiltin<"stdlib.h"> {
+  let Spellings = ["abs"];
+  let Attributes = [NoThrow, Const];
+  let Prototype = "T(T)";
+  let AddBuiltinPrefixedAlias = 1;
+}
+
 def Calloc : LibBuiltin<"stdlib.h"> {
   let Spellings = ["calloc"];
   let Prototype = "void*(size_t, size_t)";
@@ -3073,38 +3096,38 @@ def MemAlign : GNULibBuiltin<"malloc.h"> {
 
 // POSIX string.h
 
-def MemcCpy : GNULibBuiltin<"stdlib.h"> {
+def MemcCpy : GNULibBuiltin<"string.h"> {
   let Spellings = ["memccpy"];
   let Prototype = "void*(void*, void const*, int, size_t)";
 }
 
-def MempCpy : GNULibBuiltin<"stdlib.h"> {
+def MempCpy : GNULibBuiltin<"string.h"> {
   let Spellings = ["mempcpy"];
   let Prototype = "void*(void*, void const*, size_t)";
 }
 
-def StpCpy : GNULibBuiltin<"stdlib.h"> {
+def StpCpy : GNULibBuiltin<"string.h"> {
   let Spellings = ["stpcpy"];
   let Attributes = [NoThrow];
   let Prototype = "char*(char*, char const*)";
   let AddBuiltinPrefixedAlias = 1;
 }
 
-def StpnCpy : GNULibBuiltin<"stdlib.h"> {
+def StpnCpy : GNULibBuiltin<"string.h"> {
   let Spellings = ["stpncpy"];
   let Attributes = [NoThrow];
   let Prototype = "char*(char*, char const*, size_t)";
   let AddBuiltinPrefixedAlias = 1;
 }
 
-def StrDup : GNULibBuiltin<"stdlib.h"> {
+def StrDup : GNULibBuiltin<"string.h"> {
   let Spellings = ["strdup"];
   let Attributes = [NoThrow];
   let Prototype = "char*(char const*)";
   let AddBuiltinPrefixedAlias = 1;
 }
 
-def StrnDup : GNULibBuiltin<"stdlib.h"> {
+def StrnDup : GNULibBuiltin<"string.h"> {
   let Spellings = ["strndup"];
   let Attributes = [NoThrow];
   let Prototype = "char*(char const*, size_t)";
@@ -3274,22 +3297,22 @@ def ObjcEnumerationMutation : ObjCLibBuiltin<"objc/runtime.h"> {
   let Prototype = "void(id)";
 }
 
-def ObjcReadWeak : ObjCLibBuiltin<"objc/message.h"> {
+def ObjcReadWeak : ObjCLibBuiltin<"objc/objc-auto.h"> {
   let Spellings = ["objc_read_weak"];
   let Prototype = "id(id*)";
 }
 
-def ObjcAssignWeak : ObjCLibBuiltin<"objc/message.h"> {
+def ObjcAssignWeak : ObjCLibBuiltin<"objc/objc-auto.h"> {
   let Spellings = ["objc_assign_weak"];
   let Prototype = "id(id, id*)";
 }
 
-def ObjcAssignIvar : ObjCLibBuiltin<"objc/message.h"> {
+def ObjcAssignIvar : ObjCLibBuiltin<"objc/objc-auto.h"> {
   let Spellings = ["objc_assign_ivar"];
   let Prototype = "id(id, id, ptrdiff_t)";
 }
 
-def ObjcAssignGlobal : ObjCLibBuiltin<"objc/message.h"> {
+def ObjcAssignGlobal : ObjCLibBuiltin<"objc/objc-auto.h"> {
   let Spellings = ["objc_assign_global"];
   let Prototype = "id(id, id*)";
 }
@@ -3359,13 +3382,6 @@ def Atan2 : FPMathTemplate, LibBuiltin<"math.h"> {
   let AddBuiltinPrefixedAlias = 1;
 }
 
-def Abs : IntMathTemplate, LibBuiltin<"math.h"> {
-  let Spellings = ["abs"];
-  let Attributes = [NoThrow, Const];
-  let Prototype = "T(T)";
-  let AddBuiltinPrefixedAlias = 1;
-}
-
 def Copysign : FPMathTemplate, LibBuiltin<"math.h"> {
   let Spellings = ["copysign"];
   let Attributes = [NoThrow, Const];
@@ -4024,7 +4040,7 @@ def Move : CxxLibBuiltin<"utility"> {
   let Namespace = "std";
 }
 
-def MoveIfNsoexcept : CxxLibBuiltin<"memory"> {
+def MoveIfNsoexcept : CxxLibBuiltin<"utility"> {
   let Spellings = ["move_if_noexcept"];
   let Attributes = [NoThrow, Const, IgnoreSignature, RequireDeclaration,
                     Constexpr];

@michele-scandale
Copy link
Contributor Author

There are still some differences that I haven't addressed here as they might be ok to have:

  • __builtin_rint{,f,l} were defined with the const attribute, now they use the const when FP exceptions are ignored -- this seems due to the definition of these has been merged with the corresponding library builtins rint{,f,l}.
  • __builtin_{wcschr,wcscmp,wcslen,wcsncmp} and corresponding library builtins now are marked pure
  • __addressof was language builtin, now is a library builtin (like addressof)
  • finite{,f,l} were defined with GNU_LANG property, now they are defined with the ALL_GNU_LANGUAGES property
  • __builtin_va_{copy,end} are now marked with the property that they are a libc/libm function with a '__builtin_' prefix added
  • a bunch of C library builtins are now marked nothrow (e.g. abort, alloca, bcopy, bzero, index, fprintf, memchr, memcpy, memmove, memset, rindex, snprintf, sprintf, stpcpy stpncpy, strcat, strchr, strcmp, strcpy, strdup, strlen, strncat, strncmp, strncpy, strpbrk, strrchr, strspn, strstr, vfprintf, vprintf, vsnprintf, vsprintf, wcschr, wcscmp, wcslen, wcsncmp, wmemchr, wmemcmp, wmemcpy, wmemmove
  • snprintf, sprintf, vfprintf, vprintf, vsnprintf, vsprintf have now pointer arguments marked restrict.

Any comment?

@michele-scandale michele-scandale changed the title [Clang] Fixes of builtins definitions after PR #68324. [Clang] Fixes of builtin definitions after PR #68324. Feb 7, 2024
@AaronBallman
Copy link
Collaborator

There are still some differences that I haven't addressed here as they might be ok to have:

* `__builtin_rint{,f,l}` were defined with the `const` attribute, now they use the `const when FP exceptions are ignored` -- this seems due to the definition of these has been merged with the corresponding library builtins `rint{,f,l}`.

* `__builtin_{wcschr,wcscmp,wcslen,wcsncmp}` and corresponding library builtins now are marked `pure`

* `__addressof` was language builtin, now is a library builtin (like `addressof`)

* `finite{,f,l}` were defined with `GNU_LANG` property, now they are defined with the `ALL_GNU_LANGUAGES` property

* `__builtin_va_{copy,end}` are now marked with the property that they are a `libc/libm function with a '__builtin_' prefix added`

* a bunch of C library builtins are now marked `nothrow` (e.g. `abort`, `alloca`, `bcopy`, `bzero`, `index`, `fprintf`, `memchr`, `memcpy`, `memmove`, `memset`, `rindex`, `snprintf`, `sprintf`, `stpcpy` `stpncpy`, `strcat`, `strchr`, `strcmp`, `strcpy`, `strdup`, `strlen`, `strncat`, `strncmp`, `strncpy`, `strpbrk`, `strrchr`, `strspn`, `strstr`, `vfprintf`, `vprintf`, `vsnprintf`, `vsprintf`, `wcschr`, `wcscmp`, `wcslen`, `wcsncmp`, `wmemchr`, `wmemcmp`, `wmemcpy`, `wmemmove`

* `snprintf`, `sprintf`, `vfprintf`, `vprintf`, `vsnprintf`, `vsprintf` have now pointer arguments marked `restrict`.

Any comment?

Thank you for catching these mistakes! I'm of two minds: some of these changes seem valid and correct, but I don't think they were intended. For example, printf family functions now having pointers marked restrict maps to the standard signature for them and seems like a good change. But a change like with __addressof may be more questionable. Perhaps we should restore the old behavior in all cases, then re-land the good changes in follow-ups. That's more effort now, but it's easier for people to track down why a change was made in a few years. WDYT?

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, these fixes all look correct to me. The only thing that seemed somewhat off was with the sync functions taking a pointer to a volatile (that's not documented in GCC's docs, but I think the signature does make sense). Thank you for the fixes!

@michele-scandale
Copy link
Contributor Author

michele-scandale commented Feb 12, 2024

There are still some differences that I haven't addressed here as they might be ok to have:

* `__builtin_rint{,f,l}` were defined with the `const` attribute, now they use the `const when FP exceptions are ignored` -- this seems due to the definition of these has been merged with the corresponding library builtins `rint{,f,l}`.

* `__builtin_{wcschr,wcscmp,wcslen,wcsncmp}` and corresponding library builtins now are marked `pure`

* `__addressof` was language builtin, now is a library builtin (like `addressof`)

* `finite{,f,l}` were defined with `GNU_LANG` property, now they are defined with the `ALL_GNU_LANGUAGES` property

* `__builtin_va_{copy,end}` are now marked with the property that they are a `libc/libm function with a '__builtin_' prefix added`

* a bunch of C library builtins are now marked `nothrow` (e.g. `abort`, `alloca`, `bcopy`, `bzero`, `index`, `fprintf`, `memchr`, `memcpy`, `memmove`, `memset`, `rindex`, `snprintf`, `sprintf`, `stpcpy` `stpncpy`, `strcat`, `strchr`, `strcmp`, `strcpy`, `strdup`, `strlen`, `strncat`, `strncmp`, `strncpy`, `strpbrk`, `strrchr`, `strspn`, `strstr`, `vfprintf`, `vprintf`, `vsnprintf`, `vsprintf`, `wcschr`, `wcscmp`, `wcslen`, `wcsncmp`, `wmemchr`, `wmemcmp`, `wmemcpy`, `wmemmove`

* `snprintf`, `sprintf`, `vfprintf`, `vprintf`, `vsnprintf`, `vsprintf` have now pointer arguments marked `restrict`.

Any comment?

Thank you for catching these mistakes! I'm of two minds: some of these changes seem valid and correct, but I don't think they were intended. For example, printf family functions now having pointers marked restrict maps to the standard signature for them and seems like a good change. But a change like with __addressof may be more questionable. Perhaps we should restore the old behavior in all cases, then re-land the good changes in follow-ups. That's more effort now, but it's easier for people to track down why a change was made in a few years. WDYT?

The cases (1), (2), (5), (6), (7) seem the result of merging the builtin with the library builtins definitions. I don't think there is anything wrong with such changes, hence undoing the merge here, and in a separate change reverting that part doesn't seem very useful.

About (3) I have no idea if there is an actual difference in behavior w.r.t. its application. Reverting it seems straight forward.

About (4) I believe the difference isn't harmful considering that GNU_LANG is a subset of ALL_GNU_LANGUAGES.

@michele-scandale
Copy link
Contributor Author

Added the change to restore __addressof to the prior definition. To do that I have to introduce the FunctionWithoutBuiltinPrefix attribute (expanded to f) -- this is implicitly added for builtins deriving from LibBuiltin, but for this case we are expecting to declare __addressof as a language builtin.

@michele-scandale
Copy link
Contributor Author

@AaronBallman, ping.

1 similar comment
@michele-scandale
Copy link
Contributor Author

@AaronBallman, ping.

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for the fix!

This commit addresses few differences between the `Builtins.def` file
before the refactoring done in PR llvm#68324 and the currently generated
`Builtins.inc` file.
@michele-scandale michele-scandale force-pushed the fix-clang-builtins-tablegen branch from 3854236 to 016d9f1 Compare March 4, 2024 18:23
@michele-scandale michele-scandale merged commit 57592e9 into llvm:main Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants