Page MenuHomePhabricator

[CUDA] Emit host-side 'shadows' for device-side global variables
ClosedPublic

Authored by tra on Mar 1 2016, 12:55 PM.

Details

Summary

.. and register them with CUDA runtime.

This is needed for commonly used cudaMemcpy*() APIs that use address of host-side shadow to access their counterparts on device side.

Fixes PR26340.

Event Timeline

tra updated this revision to Diff 49530.Mar 1 2016, 12:55 PM
tra retitled this revision from to [CUDA] Emit host-side 'shadows' for device-side global variables.
tra updated this object.
tra added reviewers: jlebar, jingyue.
tra added a subscriber: cfe-commits.
jlebar added inline comments.Mar 1 2016, 2:38 PM
lib/CodeGen/CGCUDANV.cpp
168 ↗(On Diff #49561)

This is kind of hard to parse. How about rephrasing to something like:

Creates a function that sets up state on the host side for CUDA objects that have a presence on both the host and device sides. Specifically, registers the host side of kernel functions and device global variables with the CUDA runtime.

240 ↗(On Diff #49561)

Can we say what these args mean?

251 ↗(On Diff #49561)

Nit: s/args/Args/?

255 ↗(On Diff #49561)

Nit: Maybe pull this expression out as a separate var? Then the comment isn't needed (would be nice, because at the moment it's ambiguous exactly what "sizeof(var)" refers to.

lib/CodeGen/CodeGenModule.cpp
1532 ↗(On Diff #49561)

s/CUDA runtime API/the CUDA runtime/ (not really a requirement of the API, I think?)

1584 ↗(On Diff #49561)

Kind of an odd way of writing this control flow? Could we phrase it more idiomatically as

MustEmitForCUDA = !VD->hasDefinition() && ...;
if (!MustEmitForCUDA && ...) return;
2480 ↗(On Diff #49561)

Is it worth explaining why the shadows get internal linkage?

2483 ↗(On Diff #49561)

with the CUDA runtime

2486 ↗(On Diff #49561)

Now that I see them in context, I think these flags would be a lot easier to handle if they employed less abbreviation. "ExternalDeviceVar", "ConstDeviceVar"?

test/CodeGenCUDA/device-stub.cu
14 ↗(On Diff #49561)

Not sure if this is a typo or if you mean "...".

17 ↗(On Diff #49561)

Here you do seem to mean "..."

tra updated this revision to Diff 49561.Mar 1 2016, 4:04 PM
tra marked 9 inline comments as done.

Addressed Justin's comments.

jlebar accepted this revision.Mar 1 2016, 5:09 PM
jlebar edited edge metadata.
This revision is now accepted and ready to land.Mar 1 2016, 5:09 PM
This revision was automatically updated to reflect the committed changes.