Page MenuHomePhabricator

[CUDA] Raise an error if a wrong-side call is codegen'ed.
ClosedPublic

Authored by jlebar on Aug 6 2016, 12:40 PM.

Details

Summary

Some function calls in CUDA are allowed to appear in
semantically-correct programs but are an error if they're ever
codegen'ed. Specifically, a host+device function may call a host
function, but it's an error if such a function is ever codegen'ed in
device mode (and vice versa).

Previously, clang made no attempt to catch these errors. For the most
part, they would be caught by ptxas, and reported as "call to unknown
function 'foo'".

Now we catch these errors and report them the same as we report other
illegal calls (e.g. a call from a host function to a device function).

This has a small change in error-message behavior for calls that were
previously disallowed (e.g. calls from a host to a device function).
Previously, we'd catch disallowed calls fairly early, before doing
additional semantic checking e.g. of the call's arguments. Now we catch
these illegal calls at the very end of our semantic checks, so we'll
only emit a "illegal CUDA call" error if the call is otherwise
well-formed.

Diff Detail

Repository
rL LLVM

Event Timeline

jlebar updated this revision to Diff 67083.Aug 6 2016, 12:40 PM
jlebar retitled this revision from to [CUDA] Raise an error if a wrong-side call is codegen'ed..
jlebar updated this object.
jlebar added reviewers: tra, rnk.
jlebar added a subscriber: cfe-commits.
tra accepted this revision.Aug 8 2016, 2:09 PM
tra edited edge metadata.

Few nits, but looks good otherwise.
Should be add few tests for calling device functions from host-side global initializers? Perhaps for device->host, too, as there may be unexpected interplay with constructor emptiness checks.

clang/include/clang/Sema/Sema.h
9162 ↗(On Diff #67083)

\p Callee

clang/lib/Sema/SemaCUDA.cpp
493 ↗(On Diff #67083)

Perhaps we should add assert(Callee) before we use it.

clang/lib/Sema/SemaDeclCXX.cpp
11510–11511 ↗(On Diff #67083)

Single if would do here.

clang/lib/Sema/SemaExpr.cpp
5119 ↗(On Diff #67083)

static?

This revision is now accepted and ready to land.Aug 8 2016, 2:09 PM
jlebar marked 3 inline comments as done.Aug 9 2016, 10:52 AM

Should be add few tests for calling device functions from host-side global initializers? Perhaps for device->host, too, as there may be unexpected interplay with constructor emptiness checks.

Hm, these seem completely broken, before and after this patch. I'll handle it separately if that's OK.

clang/include/clang/Sema/Sema.h
9162 ↗(On Diff #67083)

This file isn't at all consistent about using this. For example, we're missing three \ps on IsAllowedCUDACall, two on IdentifyCUDAPreference just above, and four on maybeAddCUDAHostDeviceAttrs.

If we were to do it here, we'd need to add \p to three places; not just Callee, but also CFP_Never and CFP_WrongSide.

I'm highly skeptical that this ends up being beneficial overall to readers (i.e. that the clutter in source code outweighs the slightly prettier formatting on doxygen).

jlebar updated this revision to Diff 67378.Aug 9 2016, 11:13 AM
jlebar edited edge metadata.

Address review comments.

Also swap the two tests -- I had the wrong names on them.

jlebar added a comment.Aug 9 2016, 3:12 PM

Hm, these seem completely broken, before and after this patch. I'll handle it separately if that's OK.

D23335. The device side equivalent works afaict (and is very well-tested).

rnk accepted this revision.Aug 15 2016, 3:45 PM
rnk edited edge metadata.

lgtm

clang/include/clang/Sema/Sema.h
9162 ↗(On Diff #67378)

FWIW I never insert doxygen annotations. I figure if someone cares they can come insert them for me. =P

This revision was automatically updated to reflect the committed changes.