This is an archive of the discontinued LLVM Phabricator instance.

[CUDA] Add #pragma clang force_cuda_host_device_{begin,end} pragmas.
ClosedPublic

Authored by jlebar on Sep 27 2016, 11:41 AM.

Details

Summary

These cause us to consider all functions in-between to be host
device.

You can nest these pragmas; you just can't have more 'end's than
'begin's.

Diff Detail

Repository
rL LLVM

Event Timeline

jlebar updated this revision to Diff 72682.Sep 27 2016, 11:41 AM
jlebar retitled this revision from to [CUDA] Add #pragma clang force_cuda_host_device_{begin,end} pragmas..
jlebar updated this object.
jlebar added a reviewer: rsmith.
jlebar added subscribers: cfe-commits, jhen, tra.
tra added a comment.Sep 27 2016, 12:38 PM

LGTM. Should we add new pragma description to docs/LanguageExtensions.rst ?

rsmith added inline comments.Sep 27 2016, 2:07 PM
clang/lib/Parse/ParsePragma.cpp
172 ↗(On Diff #72682)

I think this would be more consistent with the behavior of our other pragmas as two tokens rather than one:

#pragma clang force_cuda_host_device begin
#pragma clang force_cuda_host_device end

It's also more extensible this way, for instance if we later want some kind of push/pop mechanism to temporarily leave force-host-device mode. (Maybe we could even use #pragma clang cuda force_host_device begin, but I'm fine with either choice there.)

clang/lib/Sema/SemaCUDA.cpp
466–472 ↗(On Diff #72682)

Does this apply within template instantiations? I'm concerned about two cases:

  1. Eagerly-instantiated declarations whose instantiation is triggered within one of these scopes:
template<typename T> auto f() { return T(); }
template<typename T> struct X { void f(); };
#pragma clang force_cuda_host_device_begin
int n = f<int>(); // is f<int> implicitly __host__ __device__?
X<int> x; // is X<int>::f implicitly __host__ __device__?
#pragma clang force_cuda_host_device_end
  1. Template instantiation at the end of the TU if we have a begin but no end:
template<typename T> int f() { return T(); }
#pragma clang force_cuda_host_device_begin
int n = f<int>(); // is f<int> implicitly __host__ __device__?

I would expect the answer in each case to be that the function is host. Even if these do work correctly already, please add some testcases.

jlebar updated this revision to Diff 72717.Sep 27 2016, 3:00 PM
jlebar marked 2 inline comments as done.

Address Richard Smith's review comments:

  • Change macro format.
  • Add tests (these Just Worked).
rsmith edited edge metadata.Sep 27 2016, 3:39 PM

Please add serialisation code for the push count for PCH. Should it be an error if the count is nonzero at the end of the TU?

What happens if there are trailing tokens after the pragma?

clang/include/clang/Basic/DiagnosticParseKinds.td
1027 ↗(On Diff #72717)

Diagnostics should start with a lowercase letter.

clang/test/Parser/cuda-force-host-device-templates.cu
8 ↗(On Diff #72717)

You need this to return auto to trigger the eager instantiation codepath I was concerned about.

jlebar updated this revision to Diff 72734.Sep 27 2016, 4:55 PM
jlebar marked 2 inline comments as done.
jlebar edited edge metadata.

Address Richard's comments.

I'm fairly neutral on whether we want to make it an error not to match all of
your "begin" pragmas with "end"s. I checked pragma push_macro, and it looks
like it's not an error to pop those, so with that prior art, and since it was
simpler not to check for matching begin/ends, I did the same. But like I say,
I don't feel strongly either way (or even if we wanted to make these new
pragmas not-nestable).

What happens if there are trailing tokens after the pragma?

Added code to make this an error.

rsmith accepted this revision.Oct 7 2016, 4:25 PM
rsmith edited edge metadata.

Please add a test to test/PCH for the serialization code. Otherwise, LGTM.

clang/include/clang/Basic/DiagnosticParseKinds.td
1032 ↗(On Diff #72734)

Diagnostics should not end with a period. =)

clang/include/clang/Basic/DiagnosticSemaKinds.td
6701–6702 ↗(On Diff #72734)

This appears to be unused.

This revision is now accepted and ready to land.Oct 7 2016, 4:25 PM
jlebar marked 2 inline comments as done.Oct 8 2016, 3:22 PM

Please add a test to test/PCH for the serialization code. Otherwise, LGTM.

Test added. It caught a bug, too. :)

Thank you for the review.

This revision was automatically updated to reflect the committed changes.