Page MenuHomePhabricator

[OpenCL] Add distinct file extension for C++ for OpenCL
ClosedPublic

Authored by Anastasia on Feb 16 2021, 5:20 AM.

Details

Summary

Files compiled with C++ for OpenCL can have a distinct file extension - clcpp, then clang driver will pick the compilation mode automatically without the use of -cl-std=clc++

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

Diff Detail

Event Timeline

Anastasia created this revision.Feb 16 2021, 5:20 AM
Anastasia requested review of this revision.Feb 16 2021, 5:20 AM

I feel this would be a valuable addition, indeed. Only a minor question from me.

clang/lib/Driver/Types.cpp
252

I'm not sure we want that -- I'm actually fine if we don't -- but I see below c++ and cxx are supported in addition to cpp. Should we therefore also have clc++ and clcxx as file valid extensions for consistency? I wonder what the general opinion is.

svenvh added inline comments.Feb 16 2021, 8:53 AM
clang/lib/Driver/Types.cpp
252

And there is also .cc above. Since you're asking for opinions, my personal opinion is that one extension should be enough, and providing a CL counterpart for all existing C++ file extensions does not bring more value.

Anastasia added inline comments.Feb 17 2021, 10:44 AM
clang/lib/Driver/Types.cpp
252

FYI, there are some more discussions on RFC thread https://lists.llvm.org/pipermail/cfe-dev/2021-February/067703.html

Anastasia edited the summary of this revision. (Show Details)Feb 17 2021, 10:44 AM

Hi @Anastasia , thank you for working on this!

IIUC, this patch introduces:

  • new input types: TY_CLCXX and TY_CLCXXHeader
  • new language: OpenCLCXX

Based on the attached test, this is only to remove the need for -cl-std=clc++. Do you expect any other benefits of introducing the above ^^^? And are you planning to to update the OpenCL C++ tests not to use -cl-std=clc++? Just trying to make sure I understand the reason behind this :)

clang/lib/Driver/Types.cpp
163

This is not used.

Hi @Anastasia , thank you for working on this!

IIUC, this patch introduces:

  • new input types: TY_CLCXX and TY_CLCXXHeader
  • new language: OpenCLCXX

Based on the attached test, this is only to remove the need for -cl-std=clc++. Do you expect any other benefits of introducing the above ^^^?

This is only the initial patch and for the moment the primary goal is to remove the need for the flag at least from the clang perspective.

And are you planning to to update the OpenCL C++ tests not to use -cl-std=clc++?

I was not sure about this. I just dislike the fact that moving files in the repo complicates the commit history lookup so I was not sure if the renaming was good or bad thing. Do you have any suggestions?

I was thinking to introduce as a guideline that the new tests should definitely use the new extension though.

I find the way driver is setup for the languages quite different so I was wondering if we have any guidelines?

clang/lib/Driver/Types.cpp
163

This is used in the patch that is planned to be committed hopefully soon:
https://reviews.llvm.org/D96515

jansvoboda11 resigned from this revision.Feb 19 2021, 5:10 AM

This is only the initial patch and for the moment the primary goal is to remove the need for the flag at least from the clang perspective.

Shorter compiler invocation is always nice! Does OpenCL (will) require a flag to specify the C++ standard used in the kernel? Or is that going to be controlled with -cl-std={cl2.1|cl2.2|cl2.3? This would be very weird -cl-std=clc++ -cl-std=cl2.1, so I see why you would want to remove -cl-std=clc++.

I just dislike the fact that moving files in the repo complicates the commit history lookup so I was not sure if the renaming was good or bad thing. Do you have any suggestions?

Have you tried it? Git is very clever in this respect and will track the file changes very accurately anyway. IMHO this wouldn't be disruptive in terms of browsing the history at all. If you are making this change to remove the need for -cl-std=clc++ and to simplify things, you could as well organize tests per kernel language (i.e. OpenCL C vs OpenCL C++). Also, -cl-std=clc++ in tests will look very confusing (i.e., why keep OpenCL C++ tests in OpenCL C tests and force the standard with -cl-std=clc++)? Lastly, such change would be a very through verification of your changes :)

I was thinking to introduce as a guideline that the new tests should definitely use the new extension though.

+1

I find the way driver is setup for the languages quite different so I was wondering if we have any guidelines?

I'm not aware of any documentation for the driver, sorry.

By scanning your patch I get the impression that you don't need TY_CLCXX just yet. The language for the kernel is set in clang/lib/Frontend/CompilerInvocation.cpp:

.Case("clcpp", Language::OpenCLCXX)

In Types.cpp this should be sufficient for now:

.Case("clcpp", TY_CLCXX)

Basically, you could limit your changes to the frontend driver and the changes in the compiler driver keep to a tiny minimum. I think! :)

Anastasia updated this revision to Diff 327915.Mar 3 2021, 1:58 PM
  • Renamed extensions to cppcl as it was more popular in RFC
  • Minimized number of changes in driver setup
  • Changed file extension in all C++ for OpenCL tests

This is only the initial patch and for the moment the primary goal is to remove the need for the flag at least from the clang perspective.

Shorter compiler invocation is always nice! Does OpenCL (will) require a flag to specify the C++ standard used in the kernel? Or is that going to be controlled with -cl-std={cl2.1|cl2.2|cl2.3? This would be very weird -cl-std=clc++ -cl-std=cl2.1, so I see why you would want to remove -cl-std=clc++.

Thanks a lot for all the feedback! Just to clarify I am not removing the flag I am just removing the need for it to be passed. FYI I have modified the driver test slightly to indicate that the flag is still available. The reason for it is backward compatibility. OpenCL (not only C++ for OpenCL) has always used this flag to override the default settings per file extensions. Some IDEs and other toolchains started relying on this feature. For example, they would compile .c extension file with -cl-std=cl. Not sure why it was set up this way but it is not something that would be easy to change without introducing backward compatibility issue.

I just dislike the fact that moving files in the repo complicates the commit history lookup so I was not sure if the renaming was good or bad thing. Do you have any suggestions?

Have you tried it? Git is very clever in this respect and will track the file changes very accurately anyway. IMHO this wouldn't be disruptive in terms of browsing the history at all. If you are making this change to remove the need for -cl-std=clc++ and to simplify things, you could as well organize tests per kernel language (i.e. OpenCL C vs OpenCL C++). Also, -cl-std=clc++ in tests will look very confusing (i.e., why keep OpenCL C++ tests in OpenCL C tests and force the standard with -cl-std=clc++)? Lastly, such change would be a very through verification of your changes :)

Yes, I agree. I have renamed the tests. The only drawback is that git log would need an extra option to see the full history but it should be acceptable and I am up for consistency. Also if we are ever to rename those tests now is better than later.

By scanning your patch I get the impression that you don't need TY_CLCXX just yet. The language for the kernel is set in clang/lib/Frontend/CompilerInvocation.cpp:

.Case("clcpp", Language::OpenCLCXX)

In Types.cpp this should be sufficient for now:

.Case("clcpp", TY_CLCXX)

Basically, you could limit your changes to the frontend driver and the changes in the compiler driver keep to a tiny minimum. I think! :)

Yes, I have removed most of the redundant code. It was mainly related to the headers btw. It looks a lot cleaner now. :)

Anastasia edited the summary of this revision. (Show Details)Mar 5 2021, 2:55 AM
svenvh accepted this revision.Mar 10 2021, 5:32 AM

LGTM, but maybe give this another 24h before landing in case there are any remaining concerns.

This revision is now accepted and ready to land.Mar 10 2021, 5:32 AM
awarzynski accepted this revision.Mar 10 2021, 7:54 AM

Thank you for addressing my comments and for working on this! I've left one small suggestion, but that's a [nit].

clang/test/Driver/cxx_for_opencl.cppcl
1 ↗(On Diff #327915)

This is a very neat test. Would it make sense to add something more basic too? For example (in a separate file):

// RUN: %clang %s -fstynax-only -### | FileCheck %s

// CHECK: -x -cppcl

This way you would have a bit more explicit test to verifiy that the compiler driver picks the right language based on the extension.

Anastasia updated this revision to Diff 330698.Mar 15 2021, 9:53 AM

Updated Driver test to check -x option.

I had a second thought about the extension name and I realized that the reason why I initially wanted to use clcpp is that it aligns better with clc++ which is used in -cl-std. Even though from the RFC the preference was towards cppcl it felt like there was no objection to clcpp either. So I just want to check one last time whether it would make sense to align with clc++ and use clcpp. Perhaps, it make clang interface a bit more inconsistent?

clang/test/Driver/cxx_for_opencl.cppcl
1 ↗(On Diff #327915)

Good idea! Thanks!

I had a second thought about the extension name and I realized that the reason why I initially wanted to use clcpp is that it aligns better with clc++ which is used in -cl-std. Even though from the RFC the preference was towards cppcl it felt like there was no objection to clcpp either. So I just want to check one last time whether it would make sense to align with clc++ and use clcpp. Perhaps, it make clang interface a bit more inconsistent?

That is a good point, in my opinion aligning with the -cl-std flag is a good reason to go for .clcpp. I would recommend mentioning this on the RfC thread too, to ensure that people who suggested a slight preference for the other extension are okay with .clcpp too.

I had a second thought about the extension name and I realized that the reason why I initially wanted to use clcpp is that it aligns better with clc++ which is used in -cl-std. Even though from the RFC the preference was towards cppcl it felt like there was no objection to clcpp either. So I just want to check one last time whether it would make sense to align with clc++ and use clcpp. Perhaps, it make clang interface a bit more inconsistent?

That is a good point, in my opinion aligning with the -cl-std flag is a good reason to go for .clcpp. I would recommend mentioning this on the RfC thread too, to ensure that people who suggested a slight preference for the other extension are okay with .clcpp too.

+1 for consistency.

Back to .clcpp!

Thanks, I have changed it to '.clcpp' and also replied in RFC: https://lists.llvm.org/pipermail/cfe-dev/2021-March/067936.html

Let's wait a few days for some more feedback.

Anastasia edited the summary of this revision. (Show Details)Mar 17 2021, 10:51 AM
svenvh accepted this revision.Mar 22 2021, 5:28 AM

LGTM; just one minor comment, which you can address as part of your commit.

clang/test/CodeGenOpenCLCXX/addrspace-with-class.clcpp
1–2

Nitpick: this -cl-std can be removed too.

This revision was landed with ongoing or failed builds.Mar 24 2021, 6:07 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2021, 6:07 AM
Herald added a subscriber: ldrumm. · View Herald Transcript

Could you also update the switch statement in LLDB here: llvm-project/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:472 ?

This looks great! Thanks for fixing this!