Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[SPIR-V 1.4] Support OpPtrEqual, OpPtrNotEqual and OpPtrDiff to compare pointers #2482
[SPIR-V 1.4] Support OpPtrEqual, OpPtrNotEqual and OpPtrDiff to compare pointers #2482
Changes from 4 commits
c7af03f
f6493a5
c9f25db
1774b5a
5ca1095
9a53933
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a nice occasion to improve diagnostics in this place and add precise info about op code that is not supported. I've once hit this exactly line when SPIRV Backend generated code for missed OpPtrEqual/NotEqual and spent some time on debugging. To avoid this in future we may want to improve diagnostics right in this place and give a user-friendly description of what exactly SPIRV Translator doesn't have support for.
@MrSidims @asudarsa what do you think? ^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with Slava
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the subsequent change from @vmaksimo does provide more information. One issue I see here is our reliance on 'assert'. Should we try to provide an error mechanism which will stop translation in such cases? This can be made as a separate effort.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a small test description (one-liner). Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a small test description (one-liner). Thanks