This is an archive of the discontinued LLVM Phabricator instance.

[CUDA] Copy host builtin types to NVPTXTargetInfo.
ClosedPublic

Authored by jlebar on Apr 20 2016, 6:02 PM.

Details

Summary

Host and device types must match, otherwise when we pass values back and
forth between the host and device, we will get the wrong result.

This patch makes NVPTXTargetInfo inherit most of its type information
from the host's target info.

Diff Detail

Event Timeline

jlebar updated this revision to Diff 54446.Apr 20 2016, 6:02 PM
jlebar retitled this revision from to [CUDA] Copy host builtin types to NVPTXTargetInfo..
jlebar updated this object.
jlebar added a reviewer: rsmith.
jlebar added subscribers: tra, jhen, cfe-commits.

Richard, friendly ping?

tra added inline comments.Apr 28 2016, 2:57 PM
lib/Basic/Targets.cpp
1642

You may want to make sure we don't recurse here if someone specifies host triple to be one of NVPTX variants.

rsmith added inline comments.Apr 28 2016, 3:10 PM
lib/Basic/Targets.cpp
1648–1684

I think we need to do more to make sure this list of fields is kept up to date as more of these values are added or changed.

Possible idea: could we have a test that builds with -E -dM in both host and device mode, greps for the __*_TYPE__, __*_MAX__ (etc) predefines, and compares them across the two runs?

jlebar updated this revision to Diff 55646.Apr 29 2016, 12:25 PM

Add more comprehensive test.

jlebar updated this revision to Diff 55648.Apr 29 2016, 12:40 PM

Add test for host == nvptx.

jlebar marked 2 inline comments as done.EditedApr 29 2016, 12:40 PM

<strike>Richard, friendly ping?</strike>

(Sorry, not sure why phabricator decided to repeat my last comment.)

lib/Basic/Targets.cpp
1642

Done and added a test, thank you for noticing this.

1648–1684

OK, done. I did TYPE, MAX, SIZEOF, and WIDTH, and I'm checking i386, x86_64, and ppc.

rsmith accepted this revision.Apr 29 2016, 3:24 PM
rsmith edited edge metadata.
rsmith added inline comments.
test/Preprocessor/cuda-types.cu
8–10

I expect you'll also need a REQUIRES: shell for this stuff.

This revision is now accepted and ready to land.Apr 29 2016, 3:24 PM
This revision was automatically updated to reflect the committed changes.
jlebar marked 2 inline comments as done.