[clang] Refactor Builtins.def to be a tablegen file #68324

philnik777 · 2023-10-05T15:36:29Z

This makes the builtins list quite a bit more verbose, but IMO this is a huge win in terms of readability.

clang/utils/TableGen/TableGen.cpp

AaronBallman

My review may take a bit because GitHub is struggling to render the changes in Builtins.td.

However, I want to state up front just how incredibly excited I am for this refactoring. This will go a long ways towards letting us start to generate documentation for all of our builtins instead of manually having to add it to the LanguageExtensions.rst file (which is easy for folks to forget or ignore). So despite the extra complexity of using .td file, I think it's worth the complexity. Thank you for the work!

AaronBallman

I'm in favor of the changes in this patch, but precommit CI is currently failing with relevant failures that should be addressed.

AaronBallman · 2023-11-15T18:54:13Z

clang/utils/TableGen/ClangBuiltinsEmitter.cpp

@@ -0,0 +1,333 @@
+//=- ClangDiagnosticsEmitter.cpp - Generate Clang diagnostics tables -*- C++ -*-


Suggested change

//=- ClangDiagnosticsEmitter.cpp - Generate Clang diagnostics tables -*- C++ -*-

//=- ClangBuiltinsEmitter.cpp - Generate Clang builtins tables -*- C++ -*-

May need to adjust formatting.

clang/utils/TableGen/ClangBuiltinsEmitter.cpp

AaronBallman · 2023-11-15T18:56:15Z

clang/utils/TableGen/ClangBuiltinsEmitter.cpp

+    Prototype = Prototype.trim();
+    Prototype = Prototype.trim();


This seems suspicious, why two trims?

I don't know. It's probably just a leftover.

clang/include/clang/Basic/BuiltinsBase.td

github-actions · 2023-11-20T13:24:06Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 54e2749609d7114f4a48f4146cddeecf76d935a4 ff03606e7d3b96d40909716b0a958f21c6b16fd2 -- clang/utils/TableGen/ClangBuiltinsEmitter.cpp clang/include/clang/AST/Expr.h clang/include/clang/Basic/Builtins.h clang/include/clang/Basic/TargetBuiltins.h clang/lib/AST/StmtPrinter.cpp clang/lib/Basic/Builtins.cpp clang/lib/Basic/Targets/BPF.cpp clang/lib/Sema/SemaChecking.cpp clang/lib/Sema/SemaExpr.cpp clang/test/Analysis/bstring.c clang/test/CodeGen/callback_pthread_create.c clang/utils/TableGen/MveEmitter.cpp clang/utils/TableGen/TableGen.cpp clang/utils/TableGen/TableGenBackends.h

View the diff from clang-format here.

diff --git a/clang/include/clang/Basic/Builtins.h b/clang/include/clang/Basic/Builtins.h
index f955d21169..b749ca4307 100644
--- a/clang/include/clang/Basic/Builtins.h
+++ b/clang/include/clang/Basic/Builtins.h
@@ -62,7 +62,7 @@ struct HeaderDesc {
 
 namespace Builtin {
 enum ID {
-  NotBuiltin  = 0,      // This is not a builtin function.
+  NotBuiltin = 0, // This is not a builtin function.
 #define BUILTIN(ID, TYPE, ATTRS) BI##ID,
 #include "clang/Basic/Builtins.inc"
   FirstTSBuiltin
diff --git a/clang/include/clang/Basic/TargetBuiltins.h b/clang/include/clang/Basic/TargetBuiltins.h
index d5de5a286f..df7fb19135 100644
--- a/clang/include/clang/Basic/TargetBuiltins.h
+++ b/clang/include/clang/Basic/TargetBuiltins.h
@@ -83,8 +83,8 @@ namespace clang {
   namespace BPF {
   enum {
     LastTIBuiltin = clang::Builtin::FirstTSBuiltin - 1,
-  #define BUILTIN(ID, TYPE, ATTRS) BI##ID,
-  #include "clang/Basic/BuiltinsBPF.inc"
+#define BUILTIN(ID, TYPE, ATTRS) BI##ID,
+#include "clang/Basic/BuiltinsBPF.inc"
     LastTSBuiltin
   };
   }
diff --git a/clang/utils/TableGen/TableGen.cpp b/clang/utils/TableGen/TableGen.cpp
index 6c1767d541..0a07ca1e85 100644
--- a/clang/utils/TableGen/TableGen.cpp
+++ b/clang/utils/TableGen/TableGen.cpp
@@ -282,11 +282,14 @@ cl::opt<ActionType> Action(
                    "Generate riscv_vector_builtin_cg.inc for clang"),
         clEnumValN(GenRISCVVectorBuiltinSema, "gen-riscv-vector-builtin-sema",
                    "Generate riscv_vector_builtin_sema.inc for clang"),
-        clEnumValN(GenRISCVSiFiveVectorBuiltins, "gen-riscv-sifive-vector-builtins",
+        clEnumValN(GenRISCVSiFiveVectorBuiltins,
+                   "gen-riscv-sifive-vector-builtins",
                    "Generate riscv_sifive_vector_builtins.inc for clang"),
-        clEnumValN(GenRISCVSiFiveVectorBuiltinCG, "gen-riscv-sifive-vector-builtin-codegen",
+        clEnumValN(GenRISCVSiFiveVectorBuiltinCG,
+                   "gen-riscv-sifive-vector-builtin-codegen",
                    "Generate riscv_sifive_vector_builtin_cg.inc for clang"),
-        clEnumValN(GenRISCVSiFiveVectorBuiltinSema, "gen-riscv-sifive-vector-builtin-sema",
+        clEnumValN(GenRISCVSiFiveVectorBuiltinSema,
+                   "gen-riscv-sifive-vector-builtin-sema",
                    "Generate riscv_sifive_vector_builtin_sema.inc for clang"),
         clEnumValN(GenAttrDocs, "gen-attr-docs",
                    "Generate attribute documentation"),

philnik777 · 2024-01-18T05:28:52Z

clang/include/clang/AST/Expr.h

-    if (Op >= AO__opencl_atomic_load && Op <= AO__opencl_atomic_fetch_max)
+    // FIXME: Allow grouping of builtins to be able to only check >= and <=
+    if (Op >= AO__opencl_atomic_compare_exchange_strong &&
+        Op <= AO__opencl_atomic_store && Op != AO__opencl_atomic_init)


Is adding the Op != AO__opencl_atomic_init here acceptable?

I think it's fine for the moment, the FIXME comment clarifies what needs to be done to improve it in the future.

I would also be fine if you wanted to tackle it now (perhaps we should have some marking in the .td file to say "these should be grouped together as a range"?).

I'd rather land this patch and improve things in follow-ups, since the refactoring is quite large and I'd like to avoid searching for added builtins again.

I've thought about adding something similar to the InGroup<SomeGroup> we have for diagnostics, but I don't have a patch yet. Maybe something like InBuiltinRange<C11AtomicRange>. That should make it trivial to teach tablegen to group the builtins, and maybe even generate functions to check whether a builtin is in a given range.

AaronBallman

The changes LGTM, thank you for this awesome refactoring! Thank you!

Please wait to land until after we branch for Clang 18 and after CI has finish running (it'll be a race to see which happens first).

AaronBallman · 2024-01-18T13:43:49Z

clang/include/clang/AST/Expr.h

-    if (Op >= AO__opencl_atomic_load && Op <= AO__opencl_atomic_fetch_max)
+    // FIXME: Allow grouping of builtins to be able to only check >= and <=
+    if (Op >= AO__opencl_atomic_compare_exchange_strong &&
+        Op <= AO__opencl_atomic_store && Op != AO__opencl_atomic_init)


I think it's fine for the moment, the FIXME comment clarifies what needs to be done to improve it in the future.

I would also be fine if you wanted to tackle it now (perhaps we should have some marking in the .td file to say "these should be grouped together as a range"?).

mikaelholmen · 2024-01-24T14:58:47Z

Hi,

This patch changed

BUILTIN(__builtin___stpncpy_chk, "c*c*cC*zz", "nF")

into

def StpncpyChk : Builtin {
  let Spellings = ["__builtin___stpncpy_chk"];
  let Attributes = [FunctionWithBuiltinPrefix, NoThrow];
  let Prototype = "int(char*, char*, char const*, size_t, size_t)";
}

Something wrong there isn't it?
Looks like the old signature was

char*(char*,char const*, size_t, size_T)

I haven't examined all builtins so no idea if there are more errors, I found this one when testing with -D_FORTIFY_SOURCE=2

philnik777 · 2024-01-24T18:27:35Z

@mikaelholmen Thanks. I've fixed the signature in e099e7b.

mikaelholmen · 2024-01-25T05:20:42Z

@mikaelholmen Thanks. I've fixed the signature in e099e7b.

Thanks.
Seems like we're missing testcases though if a bug like that passes lit tests without errors.

michele-scandale

I've found few differences w.r.t. the previous builtins definition. If the change was intended to be a NFC then they might be considered issues.

michele-scandale · 2024-01-30T21:49:02Z

clang/include/clang/Basic/Builtins.td

+  let Prototype = "T()";
+}
+
+def Inf : Builtin, FPMathWithF16F128Template {


Before this change there was

BUILTIN(__builtin_inff16 , "x" , "ncE")

which means that the f16 variant was using _Float16 rather than __fp16.

Is this change intended?

michele-scandale · 2024-01-30T21:54:02Z

clang/include/clang/Basic/Builtins.td

+  let Prototype = "T(T, int*)";
+}
+
+def HugeVal : Builtin, FPMathWithF16F128Template {


Before this change there was

BUILTIN(__builtin_huge_valf16, "x", "ncE")

which means that the f16 variant was using _Float16 rather than __fp16.

Is this change intended?

AaronBallman · 2024-01-31T14:48:15Z

I've found few differences w.r.t. the previous builtins definition. If the change was intended to be a NFC then they might be considered issues.

Thank you for catching those; this was intended as an NFC change, so I think these are bugs.

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.

This mechanism is introduced by #68324. This refactor makes the prototype and attributes clear. Reviewers: asb, kito-cheng, philnik777, topperc, preames Reviewed By: topperc Pull Request: #80280

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.

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

fpetrogalli · 2024-03-05T06:53:31Z

@philnik777 - do you plan to use the TD definitions of the builtins anywhere else in the compiler?

Francesco

philnik777 · 2024-03-05T11:20:26Z

Not right now. I think the first thing to do is moving all the builtins to tablegen before we can make better use of it, which is going to take a while.

AaronBallman · 2024-03-05T12:30:24Z

@philnik777 - do you plan to use the TD definitions of the builtins anywhere else in the compiler?

I expect it will get used to generate documentation for builtins at some point in the future, similar to what we've done for attributes and command line options.

philnik777 requested a review from AaronBallman October 5, 2023 15:36

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:dataflow Clang Dataflow Analysis framework - https://clang.llvm.org/docs/DataFlowAnalysisIntro.html labels Oct 5, 2023

tbaederr reviewed Oct 5, 2023

View reviewed changes

clang/utils/TableGen/TableGen.cpp Outdated Show resolved Hide resolved

AaronBallman reviewed Oct 5, 2023

View reviewed changes

AaronBallman reviewed Nov 15, 2023

View reviewed changes

philnik777 force-pushed the builtins_refactor_to_tablegen branch from f9e907f to da81d45 Compare November 20, 2023 13:17

[clang] Refactor Builtins.def to be a tablegen file

e2c94d9

philnik777 force-pushed the builtins_refactor_to_tablegen branch from da81d45 to e2c94d9 Compare November 20, 2023 13:20

Merge branch 'main' into builtins_refactor_to_tablegen

4034ad9

philnik777 changed the title ~~[clang][NFC] Refactor Builtins.def to be a tablegen file~~ [clang] Refactor Builtins.def to be a tablegen file Jan 18, 2024

Try to fix the CI

867df54

philnik777 commented Jan 18, 2024

View reviewed changes

Don't remove the docs in Builtins.def

71a0d95

AaronBallman approved these changes Jan 18, 2024

View reviewed changes

Try to fix windows

ff03606

philnik777 merged commit 4a58284 into llvm:main Jan 24, 2024

philnik777 deleted the builtins_refactor_to_tablegen branch January 24, 2024 10:22

michele-scandale reviewed Jan 30, 2024

View reviewed changes

wangpc-pp mentioned this pull request Feb 1, 2024

[Clang][RISCV] Refactor builtins to TableGen #80280

Closed

michele-scandale mentioned this pull request Feb 7, 2024

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

Merged

This was referenced Feb 23, 2024

[RFC][clang][Support] Extract type parsing function. [NFCI] #82797

Open

[clang][Builtins] Parse clang extended vectors types. #83584

Merged

		@@ -0,0 +1,333 @@
		//=- ClangDiagnosticsEmitter.cpp - Generate Clang diagnostics tables -- C++ --

	//=- ClangDiagnosticsEmitter.cpp - Generate Clang diagnostics tables -- C++ --
	//=- ClangBuiltinsEmitter.cpp - Generate Clang builtins tables -- C++ --

[clang] Refactor Builtins.def to be a tablegen file #68324

[clang] Refactor Builtins.def to be a tablegen file #68324

Uh oh!

Conversation

philnik777 commented Oct 5, 2023

Uh oh!

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikaelholmen commented Jan 24, 2024

Uh oh!

philnik777 commented Jan 24, 2024

Uh oh!

mikaelholmen commented Jan 25, 2024

Uh oh!

michele-scandale left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AaronBallman commented Jan 31, 2024

Uh oh!

fpetrogalli commented Mar 5, 2024

Uh oh!

philnik777 commented Mar 5, 2024

Uh oh!

AaronBallman commented Mar 5, 2024

Uh oh!

Uh oh!

github-actions bot commented Nov 20, 2023 •

edited

Loading