Skip to content
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

[Clang][Driver] Fix --save-temps for OpenCL AoT compilation #78333

Merged
merged 1 commit into from
Jan 23, 2024

Conversation

shiltian
Copy link
Contributor

We can directly call clang -c -x cl -target amdgcn -mcpu=gfx90a test.cl -o test.o
to compile an OpenCL kernel file. However, when --save-temps is enabled, it doesn't
work because the preprocessed file (.i file) is taken as C source file when it
is fed to the front end, thus causing compilation error because those OpenCL keywords
can't be recognized. This patch fixes the issue.

Copy link

github-actions bot commented Jan 16, 2024

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

You can test this locally with the following command:
git-clang-format --diff d360963aaa90710752d684035404db80c3dc1645 5eb2678a0a6a55c2d441b322d4eaaa8fe829a30f -- clang/lib/Driver/Types.cpp
View the diff from clang-format here.
diff --git a/clang/lib/Driver/Types.cpp b/clang/lib/Driver/Types.cpp
index a7b6b9000e..fc6ccf103b 100644
--- a/clang/lib/Driver/Types.cpp
+++ b/clang/lib/Driver/Types.cpp
@@ -133,7 +133,10 @@ bool types::isAcceptedByClang(ID Id) {
 
   case TY_Asm:
   case TY_C: case TY_PP_C:
-  case TY_CL: case TY_PP_CL: case TY_CLCXX: case TY_PP_CLCXX:
+  case TY_CL:
+  case TY_PP_CL:
+  case TY_CLCXX:
+  case TY_PP_CLCXX:
   case TY_CUDA: case TY_PP_CUDA:
   case TY_CUDA_DEVICE:
   case TY_HIP:

@shiltian
Copy link
Contributor Author

I'm working on a test case.

@shiltian shiltian marked this pull request as ready for review January 20, 2024 04:47
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Jan 20, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 20, 2024

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Shilei Tian (shiltian)

Changes

We can directly call clang -c -x cl -target amdgcn -mcpu=gfx90a test.cl -o test.o
to compile an OpenCL kernel file. However, when --save-temps is enabled, it doesn't
work because the preprocessed file (.i file) is taken as C source file when it
is fed to the front end, thus causing compilation error because those OpenCL keywords
can't be recognized. This patch fixes the issue.


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

3 Files Affected:

  • (modified) clang/include/clang/Driver/Types.def (+4-2)
  • (modified) clang/lib/Driver/Types.cpp (+6-1)
  • (added) clang/test/Driver/opencl_aot_save_temps.cl (+9)
diff --git a/clang/include/clang/Driver/Types.def b/clang/include/clang/Driver/Types.def
index b889883125c4c16..f72c27e1ee70193 100644
--- a/clang/include/clang/Driver/Types.def
+++ b/clang/include/clang/Driver/Types.def
@@ -37,8 +37,10 @@
 // C family source language (with and without preprocessing).
 TYPE("cpp-output",               PP_C,         INVALID,         "i",      phases::Compile, phases::Backend, phases::Assemble, phases::Link)
 TYPE("c",                        C,            PP_C,            "c",      phases::Preprocess, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
-TYPE("cl",                       CL,           PP_C,            "cl",     phases::Preprocess, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
-TYPE("clcpp",                    CLCXX,        PP_CXX,          "clcpp",  phases::Preprocess, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
+TYPE("cl",                       CL,           PP_CL,           "cl",     phases::Preprocess, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
+TYPE("cl-cpp-output",            PP_CL,        INVALID,         "cli",    phases::Compile, phases::Backend, phases::Assemble, phases::Link)
+TYPE("clcpp",                    CLCXX,        PP_CLCXX,        "clcpp",  phases::Preprocess, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
+TYPE("clcpp-cpp-output",         PP_CLCXX,     INVALID,         "clii",   phases::Compile, phases::Backend, phases::Assemble, phases::Link)
 TYPE("cuda-cpp-output",          PP_CUDA,      INVALID,         "cui",    phases::Compile, phases::Backend, phases::Assemble, phases::Link)
 TYPE("cuda",                     CUDA,         PP_CUDA,         "cu",     phases::Preprocess, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
 TYPE("cuda",                     CUDA_DEVICE,  PP_CUDA,         "cu",     phases::Preprocess, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
diff --git a/clang/lib/Driver/Types.cpp b/clang/lib/Driver/Types.cpp
index 08df34ade7b6530..a7b6b9000e1d2b1 100644
--- a/clang/lib/Driver/Types.cpp
+++ b/clang/lib/Driver/Types.cpp
@@ -133,7 +133,7 @@ bool types::isAcceptedByClang(ID Id) {
 
   case TY_Asm:
   case TY_C: case TY_PP_C:
-  case TY_CL: case TY_CLCXX:
+  case TY_CL: case TY_PP_CL: case TY_CLCXX: case TY_PP_CLCXX:
   case TY_CUDA: case TY_PP_CUDA:
   case TY_CUDA_DEVICE:
   case TY_HIP:
@@ -181,7 +181,9 @@ bool types::isDerivedFromC(ID Id) {
   case TY_PP_C:
   case TY_C:
   case TY_CL:
+  case TY_PP_CL:
   case TY_CLCXX:
+  case TY_PP_CLCXX:
   case TY_PP_CUDA:
   case TY_CUDA:
   case TY_CUDA_DEVICE:
@@ -241,6 +243,7 @@ bool types::isCXX(ID Id) {
   case TY_PP_CXXHeaderUnit:
   case TY_ObjCXXHeader: case TY_PP_ObjCXXHeader:
   case TY_CXXModule: case TY_PP_CXXModule:
+  case TY_PP_CLCXX:
   case TY_CUDA: case TY_PP_CUDA: case TY_CUDA_DEVICE:
   case TY_HIP:
   case TY_PP_HIP:
@@ -310,7 +313,9 @@ types::ID types::lookupTypeForExtension(llvm::StringRef Ext) {
       .Case("cc", TY_CXX)
       .Case("CC", TY_CXX)
       .Case("cl", TY_CL)
+      .Case("cli", TY_PP_CL)
       .Case("clcpp", TY_CLCXX)
+      .Case("clii", TY_PP_CLCXX)
       .Case("cp", TY_CXX)
       .Case("cu", TY_CUDA)
       .Case("hh", TY_CXXHeader)
diff --git a/clang/test/Driver/opencl_aot_save_temps.cl b/clang/test/Driver/opencl_aot_save_temps.cl
new file mode 100644
index 000000000000000..d858992eb59b789
--- /dev/null
+++ b/clang/test/Driver/opencl_aot_save_temps.cl
@@ -0,0 +1,9 @@
+// RUN: %clang -x cl -c %s
+// RUN: %clang -x cl --save-temps -c %s
+
+uint3 add(uint3 a, uint3 b) {
+  ulong x = a.x + (ulong)b.x;
+  ulong y = a.y + (ulong)b.y + (x >> 32);
+  uint z = a.z + b.z + (y >> 32);
+  return (uint3)(x, y, z);
+}

Copy link
Contributor

@jhuber6 jhuber6 left a comment

Choose a reason for hiding this comment

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

You should add a test that checks the output of -ccc-print-phases and -ccc-print-bindings. There's some similar cases in openmp-offload-gpu.c tests and similar. Also use CHECK-NEXT around the following lines.

@shiltian shiltian force-pushed the fix-opencl branch 2 times, most recently from 2fb901b to 6770e41 Compare January 23, 2024 14:27
We can directly call `clang -c -x cl -target amdgcn -mcpu=gfx90a test.cl -o test.o`
to compile an OpenCL kernel file. However, when `--save-temps` is enabled, it doesn't
work because the preprocessed file (`.i` file)  is taken as C source file when it
is fed to the front end, thus causing compilation error because those OpenCL keywords
can't be recognized. This patch fixes the issue.
@shiltian shiltian merged commit dc410f9 into llvm:main Jan 23, 2024
44 of 47 checks passed
@shiltian shiltian deleted the fix-opencl branch February 14, 2024 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants