Skip to content

Do not diagnose unsupported __float128 type if it's not used in device code #943

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Fznamznon opened this issue Dec 17, 2019 · 7 comments
Closed
Assignees
Labels
upstream This change is related to upstreaming SYCL support to llorg.

Comments

@Fznamznon
Copy link
Contributor

Fznamznon commented Dec 17, 2019

Now LLORG clang emits the following diagnostics from standard headers:

/usr/lib/gcc/x86_64-linux-gnu/7.3.0/../../../../include/c++/7.3.0/bits/std_abs.h:102:7: error: __float128 is not supported on this target
  abs(__float128 __x)
      ^
/usr/lib/gcc/x86_64-linux-gnu/7.3.0/../../../../include/c++/7.3.0/bits/std_abs.h:101:3: error: __float128 is not supported on this target
  __float128

and so on.
We disabled such diagnostics for device by the following change:
#181 (comment)
Experiments show that we don't emit any diagnostics if __float128 is used in device code. IMO, that is not fully correct and this diagnostic should be delayed.

@Fznamznon Fznamznon added the upstream This change is related to upstreaming SYCL support to llorg. label Dec 17, 2019
@Fznamznon Fznamznon self-assigned this Dec 17, 2019
@Fznamznon
Copy link
Contributor Author

Okay, we can start with simple disabling of diagnostic, like is was done for CUDA for _Float16 (see https://reviews.llvm.org/D57369). Then improve it once we implement deferred diagnostics for SYCL.
@bader , do you have any objections?

@bader
Copy link
Contributor

bader commented Dec 18, 2019

No.

Okay, we can start with simple disabling of diagnostic, like is was done for CUDA for _Float16 (see https://reviews.llvm.org/D57369). Then improve it once we implement deferred diagnostics for SYCL.

How difficult is to "implement deferred diagnostics for SYCL"?

@Fznamznon
Copy link
Contributor Author

No.

Okay, we can start with simple disabling of diagnostic, like is was done for CUDA for _Float16 (see https://reviews.llvm.org/D57369). Then improve it once we implement deferred diagnostics for SYCL.

How difficult is to "implement deferred diagnostics for SYCL"?

I'll take a look. I need some time to understand how it's supposed to work.

@Fznamznon
Copy link
Contributor Author

Okay, I've learned something new about deferred diagnostics. Actually, we already have almost working implementation of this feature. But we still emit diagnostics in SemaSYCL from MarkDeviceFunction Visitor's run, because deferred diagnostics aren't fully working now.
We still do it, because we don't build device call graph for diagnostics properly. FunctionDecl which represents operator () of lambda which represents SYCL kernel object just doesn't get into the DeviceCallGraph and deferred diagnostics aren't emitted.
The problem is already brilliantly described in details by @erichkeane in https://github.com/intel/llvm/pull/654/files#r327656088 .
All we need is fix this problem in a proper way and we get working deferred diagnostics (I just tried one of Erich's suggestions and changed using of getCurFunctionDecl() to getCurLexicalContext() , but as far as I understand that works not for all cases).
I think I can try to do more experiments, fix our deferred diagnostics system and upstream it. Then we won't need to upstream patches which silent any diagnostics.

@Fznamznon
Copy link
Contributor Author

Fix and cleanups for deferred diagnostics is here #971 . Deferring of diagnostics about unsupported __float128 type added as well.

@Fznamznon Fznamznon changed the title Upstream delaying of diagnostics about unsupported types for device Do not diagnose unsupported __float128 type if it's not used in device code Feb 11, 2020
@Fznamznon
Copy link
Contributor Author

Submitted a change to disable diagnostic https://reviews.llvm.org/D74387

@Fznamznon
Copy link
Contributor Author

https://reviews.llvm.org/D74387 is submitted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upstream This change is related to upstreaming SYCL support to llorg.
Projects
None yet
Development

No branches or pull requests

2 participants