Page MenuHomePhabricator

[OpenCL] Add builtin declarations by default.
ClosedPublic

Authored by Anastasia on Feb 11 2021, 8:07 AM.

Details

Summary

See original RFC: https://lists.llvm.org/pipermail/cfe-dev/2021-February/067610.html

This patch enabled the Tablegen based declarations by default along with opencl-c-base.h containing complete list of OpenCL types. It also adds a new flag -cl-no-stdinc that disables extra header includes and declarations.

The next step would be to either add missing declarations into Tablegen header or opencl-c-base.h.

The commit message suggestion:

This patch enables the builtin function declarations by default using the Tablegen solution along with the implicit include of opencl-c-base.h header. A new flag '-cl-no-stdinc' disabling all default declarations and header includes is added. If any other mechanisms were used to include the declarations (e.g. with -Xclang -finclude-default-header) and the new default approach is not sufficient the `-cl-no-stdinc` flag has to be used to switch to the old behavior.

Diff Detail

Event Timeline

Anastasia created this revision.Feb 11 2021, 8:07 AM
Anastasia requested review of this revision.Feb 11 2021, 8:07 AM

It probably makes sense to update clang/docs/UsersManual.rst as part of this change. In particular the following sentence is no longer true after this patch: "By default the OpenCL headers are not loaded and therefore certain builtin types and most of builtin functions are not declared."

clang/include/clang/Driver/Options.td
822

Typo.

Also, this suggests the option is limited to OpenCL C, is that your intent?

Anastasia updated this revision to Diff 323362.Feb 12 2021, 9:29 AM

Changed command line option text.

It probably makes sense to update clang/docs/UsersManual.rst as part of this change. In particular the following sentence is no longer true after this patch: "By default the OpenCL headers are not loaded and therefore certain builtin types and most of builtin functions are not declared."

Yes, that's right but I think there is a bigger change that needs to be made i.e. I would completely remove -finclude-default-header and let it live on OpenCLSupport page. I would prefer a separate review for docs though.

It probably makes sense to update clang/docs/UsersManual.rst as part of this change. In particular the following sentence is no longer true after this patch: "By default the OpenCL headers are not loaded and therefore certain builtin types and most of builtin functions are not declared."

Yes, that's right but I think there is a bigger change that needs to be made i.e. I would completely remove -finclude-default-header and let it live on OpenCLSupport page. I would prefer a separate review for docs though.

FYI here is the review for the docs update: https://reviews.llvm.org/D96616

Anastasia edited the summary of this revision. (Show Details)Feb 17 2021, 9:48 AM
Anastasia edited the summary of this revision. (Show Details)
Anastasia edited the summary of this revision. (Show Details)Feb 17 2021, 9:51 AM
svenvh accepted this revision.Feb 17 2021, 9:52 AM

LGTM

This revision is now accepted and ready to land.Feb 17 2021, 9:52 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2021, 4:25 AM
thakis added a subscriber: thakis.Feb 22 2021, 4:40 AM

Looks like this breaks tests: http://45.33.8.238/linux/39988/step_8.txt

Please take a look, and revert for now if it takes a while to fix.

Looks like this breaks tests: http://45.33.8.238/linux/39988/step_8.txt

Please take a look, and revert for now if it takes a while to fix.

Thanks for pointing this out. I think the following change should fix the issue, but I am still trying to reproduce it locally as it is taking time to build.

diff --git a/clang-tools-extra/test/pp-trace/pp-trace-pragma-opencl.cpp b/clang-tools-extra/test/pp-trace/pp-trace-pragma-opencl.cpp
index 8ba26c3b7396..31f61027994f 100644
--- a/clang-tools-extra/test/pp-trace/pp-trace-pragma-opencl.cpp
+++ b/clang-tools-extra/test/pp-trace/pp-trace-pragma-opencl.cpp
@@ -1,4 +1,4 @@
-// RUN: pp-trace -callbacks '*,-FileChanged,-MacroDefined' %s -- -x cl | FileCheck --strict-whitespace %s
+// RUN: pp-trace -callbacks '*,-FileChanged,-MacroDefined' %s -- -x cl -cl-no-stdinc | FileCheck --strict-whitespace %s
 
 #pragma OPENCL EXTENSION all : disable
 #pragma OPENCL EXTENSION cl_khr_int64_base_atomics : disable

FYI, @thakis would you be able to approve the fix? Otherwise, I am also ok to revert it until I figure out how to commit the fix.

Looks like this breaks tests: http://45.33.8.238/linux/39988/step_8.txt

Please take a look, and revert for now if it takes a while to fix.

Thanks for pointing this out. I think the following change should fix the issue, but I am still trying to reproduce it locally as it is taking time to build.

diff --git a/clang-tools-extra/test/pp-trace/pp-trace-pragma-opencl.cpp b/clang-tools-extra/test/pp-trace/pp-trace-pragma-opencl.cpp
index 8ba26c3b7396..31f61027994f 100644
--- a/clang-tools-extra/test/pp-trace/pp-trace-pragma-opencl.cpp
+++ b/clang-tools-extra/test/pp-trace/pp-trace-pragma-opencl.cpp
@@ -1,4 +1,4 @@
-// RUN: pp-trace -callbacks '*,-FileChanged,-MacroDefined' %s -- -x cl | FileCheck --strict-whitespace %s
+// RUN: pp-trace -callbacks '*,-FileChanged,-MacroDefined' %s -- -x cl -cl-no-stdinc | FileCheck --strict-whitespace %s
 
 #pragma OPENCL EXTENSION all : disable
 #pragma OPENCL EXTENSION cl_khr_int64_base_atomics : disable

FYI, @thakis would you be able to approve the fix? Otherwise, I am also ok to revert it until I figure out how to commit the fix.

@klimek, I am not very familiar with clang-tools-extra, would it be possible to look at the fix for the broken test? I can create a review if needed.

Looks like this breaks tests: http://45.33.8.238/linux/39988/step_8.txt

Please take a look, and revert for now if it takes a while to fix.

Thanks for pointing this out. I think the following change should fix the issue, but I am still trying to reproduce it locally as it is taking time to build.

diff --git a/clang-tools-extra/test/pp-trace/pp-trace-pragma-opencl.cpp b/clang-tools-extra/test/pp-trace/pp-trace-pragma-opencl.cpp
index 8ba26c3b7396..31f61027994f 100644
--- a/clang-tools-extra/test/pp-trace/pp-trace-pragma-opencl.cpp
+++ b/clang-tools-extra/test/pp-trace/pp-trace-pragma-opencl.cpp
@@ -1,4 +1,4 @@
-// RUN: pp-trace -callbacks '*,-FileChanged,-MacroDefined' %s -- -x cl | FileCheck --strict-whitespace %s
+// RUN: pp-trace -callbacks '*,-FileChanged,-MacroDefined' %s -- -x cl -cl-no-stdinc | FileCheck --strict-whitespace %s
 
 #pragma OPENCL EXTENSION all : disable
 #pragma OPENCL EXTENSION cl_khr_int64_base_atomics : disable

FYI, @thakis would you be able to approve the fix? Otherwise, I am also ok to revert it until I figure out how to commit the fix.

The test is already broken, so speculatively landing that and seeing if it helps sounds fine. But let's try to not keep the build broken for hours. (Was afk for a bit, and it looks like it's still broken now that I look again.)

Looks like this breaks tests: http://45.33.8.238/linux/39988/step_8.txt

Please take a look, and revert for now if it takes a while to fix.

Thanks for pointing this out. I think the following change should fix the issue, but I am still trying to reproduce it locally as it is taking time to build.

diff --git a/clang-tools-extra/test/pp-trace/pp-trace-pragma-opencl.cpp b/clang-tools-extra/test/pp-trace/pp-trace-pragma-opencl.cpp
index 8ba26c3b7396..31f61027994f 100644
--- a/clang-tools-extra/test/pp-trace/pp-trace-pragma-opencl.cpp
+++ b/clang-tools-extra/test/pp-trace/pp-trace-pragma-opencl.cpp
@@ -1,4 +1,4 @@
-// RUN: pp-trace -callbacks '*,-FileChanged,-MacroDefined' %s -- -x cl | FileCheck --strict-whitespace %s
+// RUN: pp-trace -callbacks '*,-FileChanged,-MacroDefined' %s -- -x cl -cl-no-stdinc | FileCheck --strict-whitespace %s
 
 #pragma OPENCL EXTENSION all : disable
 #pragma OPENCL EXTENSION cl_khr_int64_base_atomics : disable

FYI, @thakis would you be able to approve the fix? Otherwise, I am also ok to revert it until I figure out how to commit the fix.

The test is already broken, so speculatively landing that and seeing if it helps sounds fine. But let's try to not keep the build broken for hours. (Was afk for a bit, and it looks like it's still broken now that I look again.)

I have just committed the fix after verifying that it indeed makes the test pass: https://github.com/llvm/llvm-project/commit/b71add9777bed67e05206fa1fdb665f3e21a13ab