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

Fri Feb 19 07:48:46 PST 2021

awarzynski added a comment.

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++`.

> 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! :)

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

https://reviews.llvm.org/D96771