Skip to content
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

Only parse body of restricted function when compiling in restricted context #144

Open
wants to merge 2 commits into
base: clang_tot_upgrade
Choose a base branch
from

Conversation

AlexVlx
Copy link
Contributor

@AlexVlx AlexVlx commented Jun 15, 2018

At the moment we still parse and semantically analyse the body of a [[cpu]] only restricted function when compiling for accelerator or, conversely, a [[hc]] only function when compiling for host. This can lead to subtle issues when e.g. using AMDGCN builtins. This avoids that behaviour by way of eviscerating functions who are in this situation, thus retaining their impact on mangling whilst not exposing guts that are target specific. The suggested solution appears reasonable, if rather yucktastic in terms of the amount of code (an alternative would be to skip such declarations completely, but that may be surprising in terms of mangling across targets); I'd be happy if anyone had a better / cleaner solution. Thanks to @sameerds for discovering this.

Copy link
Contributor

@scchan scchan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@scchan
Copy link
Contributor

scchan commented Jun 28, 2018

@AlexVlx are you still looking for feedback from @whchung and @yxsamliu

@AlexVlx
Copy link
Contributor Author

AlexVlx commented Jun 28, 2018

@scchan I wouldn't mind it, if only to avoid posthumous whinging:)

@AlexVlx
Copy link
Contributor Author

AlexVlx commented Nov 24, 2018

@scchan I believe you want to consider this in the context of the memory consumption work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants