Skip to content

[SYCL] Remove a parsing error and make it a semantic error #3158

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

Merged
merged 4 commits into from
Feb 5, 2021

Conversation

AaronBallman
Copy link
Contributor

The SYCL loop attributes can only be applied to a for, do, or while
statement. However, we would diagnose this as a parsing error rather
than a semantic error. Attribute appertainment of [[]] attributes is
handled as a semantic error to avoid potentially changing the parsing
paths in the presence of an [[]] attribute.

As part of this change, I also corrected the attribute definitions to
properly reflect that these are statement attributes and have a
specific subject list. However, Clang's automated diagnostics focus
primarily on declaration attributes, so this change is mostly for
code readability rather than correctness (for the moment).

The SYCL loop attributes can only be applied to a for, do, or while
statement. However, we would diagnose this as a parsing error rather
than a semantic error. Attribute appertainment of [[]] attributes is
handled as a semantic error to avoid potentially changing the parsing
paths in the presence of an [[]] attribute.

As part of this change, I also corrected the attribute definitions to
properly reflect that these are statement attributes and have a
specific subject list. However, Clang's automated diagnostics focus
primarily on declaration attributes, so this change is mostly for
code readability rather than correctness (for the moment).
@AaronBallman
Copy link
Contributor Author

Question: why is the CI running clang-format on test files? (That seems like a really odd decision given that the community doesn't require formatting of their test files and frequently test files can't be formatted the way clang-format wants because of the nature of some tests (like lexing and preprocessing tests).

@bader
Copy link
Contributor

bader commented Feb 4, 2021

Question: why is the CI running clang-format on test files? (That seems like a really odd decision given that the community doesn't require formatting of their test files and frequently test files can't be formatted the way clang-format wants because of the nature of some tests (like lexing and preprocessing tests).

When I added clang-format check, I took the logic from the community script. I haven't tracked what has changed since then. Could you open an issue to filter out clang/test directory, please?

@AaronBallman
Copy link
Contributor Author

Question: why is the CI running clang-format on test files? (That seems like a really odd decision given that the community doesn't require formatting of their test files and frequently test files can't be formatted the way clang-format wants because of the nature of some tests (like lexing and preprocessing tests).

When I added clang-format check, I took the logic from the community script. I haven't tracked what has changed since then. Could you open an issue to filter out clang/test directory, please?

I opened #3159, thanks for the suggestion. Perhaps I'm wrong on the community stuff, as I've not looked at their scripts in a long while, but in terms of community review process, we largely ignore formatting in test files unless the formatting is truly obnoxious for some reason.

@bader
Copy link
Contributor

bader commented Feb 4, 2021

They seem to filter test directories indeed. https://github.com/google/llvm-premerge-checks/blob/main/scripts/clang-format.ignore

Copy link
Contributor

@smanna12 smanna12 left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM!

Copy link
Contributor

@elizabethandrews elizabethandrews left a comment

Choose a reason for hiding this comment

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

I'm not very familiar with TableGen, so I'm not really sure what's going on there. But otherwise, the changes LGTM. Thanks!

@AaronBallman
Copy link
Contributor Author

I'm not very familiar with TableGen, so I'm not really sure what's going on there. But otherwise, the changes LGTM. Thanks!

The tablegen bits are so that we can have the actual subject lines in Attr.td without causing errors. My long-term plan is to start extending more of the table generated checking code that we have for declarations into also working for statements (I think I'll eventually need that for some planned OpenMP work as well).

An alternative would be for me to leave the tablegen bits out and comment out the Subject lines in Attr.td, if that's preferred.

@elizabethandrews
Copy link
Contributor

An alternative would be for me to leave the tablegen bits out and comment out the Subject lines in Attr.td, if that's preferred.

I like the increased readability, even if it lacks functionality at the moment. I don't see the harm keeping it in except for maybe confusing a developer who isn't familiar with the code base? I don't have a strong preference either way. So I leave it up to you and @premanandrao.

@AaronBallman
Copy link
Contributor Author

An alternative would be for me to leave the tablegen bits out and comment out the Subject lines in Attr.td, if that's preferred.

I like the increased readability, even if it lacks functionality at the moment. I don't see the harm keeping it in except for maybe confusing a developer who isn't familiar with the code base? I don't have a strong preference either way. So I leave it up to you and @premanandrao.

Then I'd say let's leave it in for now.

@bader bader merged commit a12c6a1 into intel:sycl Feb 5, 2021
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.

4 participants