[PATCH] D96771: [OpenCL] Add distinct file extension for C++ for OpenCL

Anastasia Stulova via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Mar 3 14:10:51 PST 2021


Anastasia added a comment.

In D96771#2574638 <https://reviews.llvm.org/D96771#2574638>, @awarzynski wrote:

> In D96771#2571855 <https://reviews.llvm.org/D96771#2571855>, @Anastasia wrote:
>
>> 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. :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96771/new/

https://reviews.llvm.org/D96771



More information about the cfe-commits mailing list