Page MenuHomePhabricator

[Frontend] Treat CUDA header files as CUDA source files
ClosedPublic

Authored by tomilov on Nov 8 2020, 9:08 AM.

Details

Summary

Clang supports compiling CUDA source files,
CUDA header files may contain CUDA specific code
that is why they have special extension, which
can be recognized by nvcc (CUDA compiler driver)
as CUDA source file.
Treat them as CUDA source files.

Diff Detail

Event Timeline

tomilov created this revision.Nov 8 2020, 9:08 AM
tomilov created this object with edit policy "Administrators".
tomilov requested review of this revision.Nov 8 2020, 9:08 AM

Similar functionality is proposed for review in git-clang-format https://reviews.llvm.org/D90780.

While this looks ok, please add the people who normally make changes to this area of the repo. Maybe someone like @tejohnson or @MaskRay

I notice @yaxunl making CUDA related change in here too (so maybe a good person to ask for an opinion)

Does it need any unit test here? adding that's its supported what does it change? i.e. what is using this?

Not sure I'm the best person to review this either. But in any case, needs a test.

You can write a clang/test/Parser/ test checking %clang_cc1 -fsyntax-only works for a .cuh file without -x cuda

tomilov updated this revision to Diff 304422.Nov 11 2020, 12:49 AM

Test added.

tomilov added a subscriber: tra.

For some reason I'm not allowed to post on the review.

I hope it is resolved now.

I'd add .cu.h and .cu.cc to the list.

@tra Never heard about and never seen such a file extensions. Can you link statistically significant amount of .cu.h or .cu.cc sources?

tomilov changed the edit policy from "Administrators" to "All Users".Nov 11 2020, 11:45 AM
tra added inline comments.Nov 11 2020, 12:45 PM
lib/Frontend/FrontendOptions.cpp
32 ↗(On Diff #304422)

I'd add .cu.h and .cu.cc to the list.

tra added a comment.Nov 11 2020, 1:00 PM

In general, I'm still not quite convinced that adding CUDA detection to header extensions is all that useful.

The problem is that CUDA compilation relies on the external SDK and that essentially means that user must at least provide the path to CUDA SDK.
Just guessing that it's a CUDA header is not sufficient for correct compilation results.
It may work if clang finds a CUDA SDK via driver's autodetection machinery (I'm not sure it kicks in when used by tooling),
but it may not necessarily be the SDK used by the compilation of CUDA sources. That may lead to subtle
differences between the data you get when the header is processed by itself vs when it's included from a CUDA source.

I'm reluctant to add something that "mostly works".

lib/Frontend/FrontendOptions.cpp
32 ↗(On Diff #304422)

It would need to be moved above the cc checks so it takes precedence over C++.

In D91034#2389637, @tra wrote:

In general, I'm still not quite convinced that adding CUDA detection to header extensions is all that useful.

I'm reluctant to add something that "mostly works".

So maybe we can close this PR without merging? My goal was to fulfill requirement # From clang/lib/Frontend/FrontendOptions.cpp, all lower case from https://reviews.llvm.org/D90780.

tra accepted this revision.Nov 12 2020, 9:37 AM

OK. For clang-format it would still be somewhat useful and it's not making things worse than they already are otherwise.

This revision is now accepted and ready to land.Nov 12 2020, 9:37 AM
MaskRay accepted this revision.Nov 12 2020, 10:03 AM

@tra @MaskRay Please, commit the changes. I am not able to do.

@tra @MaskRay Please, commit the changes. I am not able to do.

Hi, can you provide the author name and author email for the commit? :)

(BTW, the patch should be created with git diff, git format-patch, arc diff so that the +++ line should look like +++ b/clang/test/Parser/cuda-check-input-kind-assoc.cuh)

tomilov updated this revision to Diff 304905.Nov 12 2020, 10:57 AM

@MaskRay Cannot find where is the raw patch as I paste it. Actually I use git format-patch -U999999 @~ command. git show @ output looks similar. I follow these instructions.

Author: Anatoliy Tomilov <tomilovanatoliy@gmail.com>
MaskRay added inline comments.Nov 12 2020, 11:12 AM
test/Parser/cuda-check-input-kind-assoc.cuh
1 ↗(On Diff #304905)

I'll add -Werror

@tra I have checked that clang-format does not call this function. Do you think this is still needed?

tra added a comment.Nov 12 2020, 11:37 AM

@tra I have checked that clang-format does not call this function. Do you think this is still needed?

I think the idea was to make sure that clang-format is in sync what clang regarding extension->language mapping. D90780 adds "cuh" as a CUDA extension and this patch, IIUIC, intended to add similar treatment to clang.
So, you are correct that technically this patch is not necessary for clang-format.
Keeping them in sync does have a minor benefit of not raising a question why the two maps are different.

This revision was automatically updated to reflect the committed changes.