Page MenuHomePhabricator

[SYCL][OpenMP] Implement thread-local storage restriction
ClosedPublic

Authored by Fznamznon on Jun 11 2020, 3:06 AM.

Details

Summary

SYCL and OpenMP prohibits thread local storage in device code,
so this commit ensures that error is emitted for device code and not
emitted for host code when host target supports it.

Diff Detail

Event Timeline

Fznamznon created this revision.Jun 11 2020, 3:06 AM
Fznamznon added a subscriber: ABataev.

@jdoerfert , @ABataev , if OpenMP needs same diagnostic as well, I can generalize it between SYCL and OpenMP.

erichkeane accepted this revision.Jun 11 2020, 5:57 AM

Give @ABataev and @jdoerfert a day or two to review before committing.

This revision is now accepted and ready to land.Jun 11 2020, 5:57 AM
riccibruno added inline comments.
clang/lib/Sema/SemaExpr.cpp
216

Nit: The convention is auto *VD.

Fznamznon updated this revision to Diff 270135.Jun 11 2020, 7:29 AM

Fixed code style.

Fznamznon marked 2 inline comments as done.Jun 11 2020, 7:30 AM
Fznamznon added inline comments.
clang/lib/Sema/SemaExpr.cpp
216

Fixed, thanks!

OpenMP has the same restriction (no surprise I guess). Thanks for the ping!

I think we do not emit diagnosis right now: https://godbolt.org/z/srDkXZ
I think we also should diagnose this the same way, though it might be beyond the scope of this patch: https://godbolt.org/z/rRZFi4

Fznamznon marked an inline comment as done.

Generalized diagnostic between SYCL and OpenMP.

Fznamznon retitled this revision from [SYCL] Implement thread-local storage restriction to [SYCL][OpenMP] Implement thread-local storage restriction.Jun 16 2020, 10:10 AM
Fznamznon edited the summary of this revision. (Show Details)
Fznamznon added a comment.EditedJun 16 2020, 10:13 AM

OpenMP has the same restriction (no surprise I guess). Thanks for the ping!

I think we do not emit diagnosis right now: https://godbolt.org/z/srDkXZ
I think we also should diagnose this the same way, though it might be beyond the scope of this patch: https://godbolt.org/z/rRZFi4

Alright, now the first case that you pointed is diagnosed as well.
The second case is not diagnosed at all and I agree that it seems beyond the scope of this patch, because it looks like using of #pragma omp task untied doesn't trigger emission of deferred diagnostics.

jdoerfert accepted this revision.Jun 16 2020, 11:37 AM

OpenMP has the same restriction (no surprise I guess). Thanks for the ping!

I think we do not emit diagnosis right now: https://godbolt.org/z/srDkXZ
I think we also should diagnose this the same way, though it might be beyond the scope of this patch: https://godbolt.org/z/rRZFi4

Alright, now the first case that you pointed is diagnosed as well.

Nice, thanks a lot!

The second case is not diagnosed at all and I agree that it seems beyond the scope of this patch, because it looks like using of #pragma omp task untied doesn't trigger emission of deferred diagnostics.

Yeah, we need to track untied tasks first. Not as part of this though.

LGTM :)

Seems that test/OpenMP/nvptx_target_codegen.cpp is completely not formatted. If I apply suggestion from pre-merge checks, this will look like a big unrelated to this patch change and it will contradict with the whole file style.

bader added a comment.Jun 17 2020, 3:11 AM

Seems that test/OpenMP/nvptx_target_codegen.cpp is completely not formatted. If I apply suggestion from pre-merge checks, this will look like a big unrelated to this patch change and it will contradict with the whole file style.

I formatted this file here: https://github.com/llvm/llvm-project/commit/93cd4115799cefa698833ca7a2f1899243d94c77.
Could you rebase your patch, please?

Fznamznon updated this revision to Diff 271327.Jun 17 2020, 4:04 AM
Fznamznon edited the summary of this revision. (Show Details)

Rebased.

This revision was automatically updated to reflect the committed changes.