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

Add Intel specific definitions from https://github.com/KhronosGroup/S… #150

Merged

Conversation

@CLAassistant
Copy link

CLAassistant commented Mar 30, 2020

CLA assistant check
All committers have signed the CLA.

@MrSidims MrSidims force-pushed the private/MrSidims/UpstreamIntelExt branch from 51de426 to 66d9b4e Compare March 30, 2020 14:56
@MrSidims MrSidims changed the title Add Intel specific from definitions https://github.com/KhronosGroup/S… Add Intel specific definitions from https://github.com/KhronosGroup/S… Mar 31, 2020
@MrSidims MrSidims force-pushed the private/MrSidims/UpstreamIntelExt branch from 66d9b4e to 5125824 Compare March 31, 2020 09:10
@MrSidims
Copy link
Contributor Author

There are several definitions, that are not presenting in spirv.core.grammar.json, like RayFlagsShift which were removed after headers generation. Was it to be expected?

{
"opname" : "OpReadPipeBlockingINTEL",
"class" : "Pipe",
"opcode" : 5946,
Copy link
Member

Choose a reason for hiding this comment

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

I'm concerned about continuing to create gaps here. See issue #151. I think you have all the intervening consecutive opcodes reserved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind, this particular gap won't be filled by these extensions.

@MrSidims MrSidims force-pushed the private/MrSidims/UpstreamIntelExt branch from 5125824 to 35b581c Compare April 5, 2020 14:32
@MrSidims
Copy link
Contributor Author

MrSidims commented Apr 5, 2020

Left only json changes excluding generated headers.

@johnkslang
Copy link
Member

Could you revisit this? Look at avoiding gaps (there was a comment about that), resolve conflicts. Did you want me to generated the headers?

@MrSidims
Copy link
Contributor Author

Could you revisit this? Look at avoiding gaps (there was a comment about that), resolve conflicts. Did you want me to generated the headers?

I'll take a look what can be done to avoid the gaps now. The problem here is that the values present in https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/master/lib/SPIRV/libSPIRV/spirv.hpp as is.

Regarding the headers generation - I can do it myself, was just a little bit confused, that several definitions disappeared, see my comment from above.

@johnkslang
Copy link
Member

I just rebuilt headers from your other commit, a few minutes ago. It all seemed consistent and okay.

It is okay if I build headers.

One thing we want to do is reuse enumerant values (from other enums) for the opcodes. If you have any reserved that have been used for other enums, feel free to reuse them for instructions also.

@MrSidims MrSidims force-pushed the private/MrSidims/UpstreamIntelExt branch from 35b581c to dfa87b6 Compare May 29, 2020 10:09
@MrSidims
Copy link
Contributor Author

Got it. Currently we are filling the op codes interval from 5568 to 5631 with following extensions:
intel/llvm#1593
intel/llvm#1611
intel/llvm#1612

will come to the mentioned in this PR gap in the future.

Copy link
Member

@johnkslang johnkslang left a comment

Choose a reason for hiding this comment

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

Seems a couple things need to be fixed.

{
"opname" : "OpSubgroupAvcSicGetInterRawSadsINTEL",
"class" : "@exclude",
"opcode" : 5816,
Copy link
Member

Choose a reason for hiding this comment

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

It seems this created two OpSubgroupAvcSicGetInterRawSadsINTEL. Is one supposed to be an alias, or we can have just one?

Copy link
Contributor Author

@MrSidims MrSidims May 29, 2020

Choose a reason for hiding this comment

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

I suppose it's an artifact appeared after rebase, removed.

},
{
"enumerant" : "KernelAttributesINTEL",
"value" : 5892,
Copy link
Member

Choose a reason for hiding this comment

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

We want to keep these in numerical order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, thanks!

Signed-off-by: Dmitry Sidorov <[email protected]>
@MrSidims
Copy link
Contributor Author

BTW, what are the rules for updating PRs in this repository? Shall fixes be amended into the original commit before updating, or it's OK to have commits, like: "Apply suggestions" and this commits will be squashed during the merge?

@MrSidims MrSidims requested a review from johnkslang May 29, 2020 11:18
@johnkslang
Copy link
Member

Something like this should go into the repo as a single commit. I'll squash when I remember. It is sometimes important to see feedback addressed as a separate commit, just for efficiency, but better to not have that jitter in master history.

Force pushing the PR is okay, when you want to control how the actual history will look.

This one is okay as it is now.

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.

3 participants