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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
