Skip to content

[Doc] Add SPV_INTEL_non_constant_addrspace_printf spec #5227

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.

Sign up for GitHub

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

Jump to bottom
Merged
bader merged 5 commits into from
Jan 31, 2022
Merged

[Doc] Add SPV_INTEL_non_constant_addrspace_printf spec #5227

bader merged 5 commits into from
Jan 31, 2022

Conversation

@KornevNikita
Copy link
Contributor

@KornevNikita KornevNikita commented Dec 27, 2021

No description provided.

@KornevNikita KornevNikita marked this pull request as draft December 27, 2021 14:24
@KornevNikita
Copy link
Contributor Author

KornevNikita commented Dec 27, 2021

@AlexeySotkin @bashbaug @mbelicki I've marked this PR as a draft, but it is ready for review. The only thing - there is no opcode for the new capability yet, so we shouldn't merge it even if there are no concerns.

@KornevNikita KornevNikita mentioned this pull request Dec 28, 2021
Merged
@KornevNikita
Copy link
Contributor Author

KornevNikita commented Jan 12, 2022

@AlexeySotkin @bashbaug @mbelicki please take a look

@mbelicki
Copy link

mbelicki commented Jan 13, 2022

If I understand correctly the only change to the original extension is:

_format_ must be a _pointer(constant, generic, private, local, global)_ to _i8_.

i.e. allowing the format pointer to be in other than constant address space. This is aligned with what was previously discussed and supported by current implementation, but in this case I think that we also need to specify that the format string must be a string literal, known at compile time. Otherwise this extension allows any i8 pointer to be a valid printf format argument.

@KornevNikita KornevNikita marked this pull request as ready for review January 15, 2022 13:32
@KornevNikita KornevNikita requested a review from a team as a code owner January 15, 2022 13:32
@AlexeySotkin AlexeySotkin requested a review from a team January 27, 2022 14:13
AlexeySotkin
AlexeySotkin previously approved these changes Jan 27, 2022
View reviewed changes
@AlexeySotkin
Copy link
Contributor

AlexeySotkin commented Jan 27, 2022

@mbelicki , @bashbaug ping.

@bader bader changed the title Add SPV_INTEL_non_constant_addrspace_printf spec [Doc] Add SPV_INTEL_non_constant_addrspace_printf spec Jan 27, 2022
mbelicki
mbelicki previously approved these changes Jan 27, 2022
View reviewed changes
Copy link

@mbelicki mbelicki left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@AlexeySotkin AlexeySotkin requested a review from bader January 28, 2022 14:23
@AlexeySotkin AlexeySotkin added spec extension All issues/PRs related to extensions specifications Documentation Missing documentation for the code, compiler or runtime features, etc. SPIR-V Issues related to SPIRV-LLVM-Translator labels Jan 28, 2022
bader
bader previously approved these changes Jan 28, 2022
View reviewed changes
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

LGTM, just a few minor comments which can be ignored.
Please, let me know if you are willing to address them or we need to get approval from Ben to merge it.

bashbaug
bashbaug previously approved these changes Jan 28, 2022
View reviewed changes
Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

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

LGTM, I've verified that the token assignments are correct.

As I've mentioned offline I'm not thrilled with the extension and capability name, but I can't think of an name that's obviously better and I can live with the current names.

@bader bader merged commit 8250a51 into intel:sycl Jan 31, 2022
AlexeySotkin pushed a commit to KhronosGroup/SPIRV-LLVM-Translator that referenced this pull request Feb 28, 2022
@KornevNikita
Support SPV_INTEL_non_constant_addrspace_printf (#1364)
6e70d1c 
Spec: intel/llvm#5227
vmaksimo pushed a commit to vmaksimo/llvm that referenced this pull request Mar 9, 2022
@KornevNikita @vmaksimo
Support SPV_INTEL_non_constant_addrspace_printf
97f4230 
Spec: intel#5227

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@6e70d1c
svenvh pushed a commit to svenvh/SPIRV-LLVM-Translator that referenced this pull request Jun 21, 2022
@KornevNikita @svenvh
Support SPV_INTEL_non_constant_addrspace_printf (KhronosGroup#1364)
731733d 
Spec: intel/llvm#5227
(cherry picked from commit 6e70d1c)
svenvh pushed a commit to KhronosGroup/SPIRV-LLVM-Translator that referenced this pull request Jun 22, 2022
@KornevNikita @svenvh
Support SPV_INTEL_non_constant_addrspace_printf (#1364)
25ef58e 
Spec: intel/llvm#5227
(cherry picked from commit 6e70d1c)
Quetzonarch pushed a commit to Quetzonarch/SPIRV-LLVM-Translator that referenced this pull request Jul 13, 2022
@KornevNikita @Quetzonarch
Support SPV_INTEL_non_constant_addrspace_printf (KhronosGroup#1364)
08c16b4 
Spec: intel/llvm#5227
@KornevNikita KornevNikita deleted the printf-spec branch October 14, 2022 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bader bader bader left review comments

@mbelicki mbelicki mbelicki left review comments

@bashbaug bashbaug bashbaug left review comments

+1 more reviewer

@AlexeySotkin AlexeySotkin AlexeySotkin left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Documentation Missing documentation for the code, compiler or runtime features, etc. spec extension All issues/PRs related to extensions specifications SPIR-V Issues related to SPIRV-LLVM-Translator

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@KornevNikita @mbelicki @AlexeySotkin @bader @bashbaug