This is an archive of the discontinued LLVM Phabricator instance.

[CUDA] Check register names on appropriate side of cuda compilation only.
ClosedPublic

Authored by tra on Aug 11 2015, 11:08 AM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

tra updated this revision to Diff 31832.Aug 11 2015, 11:08 AM
tra retitled this revision from to [CUDA] Check register names on appropriate side of cuda compilation only..
tra updated this object.
tra added reviewers: eliben, echristo.
tra added a subscriber: cfe-commits.
eliben added inline comments.Aug 11 2015, 12:30 PM
lib/Sema/SemaDecl.cpp
5944 ↗(On Diff #31832)

Since this is a CUDA-only thing, ShouldHandleTargetErrors can perhaps be named better. The checks you added are very selective now - some diags are disabled, some are not. It's not clear why some fall under the "should handle target error" umbrella. A clearer name like MatchingCUDAMode or something of the sort, may help?

5971 ↗(On Diff #31832)

Do we plan to support this for CUDA at all? Why not disable here on top?

tra updated this revision to Diff 32611.Aug 19 2015, 2:01 PM

Removed rarely used temp var.

lib/Sema/SemaDecl.cpp
5944 ↗(On Diff #31832)

Considering it's only used in two places, I may as well use DeclAttrsMatchCUDAMode() directly, making variable naming moot and hopefully making intent somewhat clearer.

CUDA code may contain constructs that can only be validated by appropriate TargetInfo() and we currently have access only to one used during current compilation mode. We will check for those errors in another CUDA compilation pass. The error set is further restricted to error classes I've ran into in practice and can reproduce and write tests for.

5971 ↗(On Diff #31832)

It does not quite fit virtual registers model used by NVPTX, but as a feature per se I believe we should keep it enabled, because it should be OK to use it for targets where it's valid. For example, using this feature in host code on x86 should be acceptable.

eliben accepted this revision.Aug 19 2015, 2:53 PM
eliben edited edge metadata.

lgtm

This revision is now accepted and ready to land.Aug 19 2015, 2:53 PM
This revision was automatically updated to reflect the committed changes.