-
Notifications
You must be signed in to change notification settings - Fork 641
Add support for SPV_INTEL_function_variants #6211
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
Conversation
s-perron
left a comment
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've reviewed the spirv-opt changes. I'm actually surprized you do not need more changes. You add tests to spirv-opt. Mainly for the remove duplicates pass.
s-perron
left a comment
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 looked at some of the generic files. I believe you need a lot more testing. The way the extension was defined is that it created parallel opcode, but shifted the operands by 1. This means that every place that looks at the capabilities, extensions, ect. must make sure the operand indices are shift by one. I'm not sure that have been thoroughly checked or tests.
|
Thanks to the review. I fixed the issues you pointed out and added more tests. The way I identified where the index should be shifted by one is that I searched for OpCapability/Extension/EntryPoint in the source. A more thorough check can be done, though I haven't found a systematic way to do it. Could you also check the other PR I made (see this PR's description)? It's used to test conditional extensions and should be merged before this PR, otherwise the tests won't pass. I'll be on vacation for the next three weeks, if more changes are required, I'll address them after that. |
alan-baker
left a comment
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.
Validator changes are good.
s-perron
left a comment
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.
The changes to the optimizer look good. I still need to look at the code in the linker.
|
The FunctionVariants.FAddAsm test seems to be failing. That is a linker test. Can you get that test fixed, and then someone will look at the linker code. Thanks. |
dneto0
left a comment
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.
Partial review of the linking material.
I'm up to about line 200 in link/fnvar.cpp but have read the other parts of the linker changes.
|
I rebased this PR onto the latest main which has the inline assembly patch merged. This should fix the FunctionVariants.FAddAsm (all tests pass on my machine). |
5dd7f3b to
05333ad
Compare
|
Also, thanks for your patience. Between the three reviewers so far I think we've approved all parts of this MR. |
|
Thanks for the thorough review! |
|
This broke GN builds in downstream projects. E.g. vulkan-deps into dawn. https://dawn-review.googlesource.com/c/dawn/+/257354 (Not blaming anyone: GN is niche, and sometimes hard to get right) |
PR #6211 added new source files but did not update BUILD.gn
This PR:
NOTE: This PR requires #6210 for the tests to pass because inline assembly is used to test conditional extensions.
The values to pass to
OpSpecConstantArchitectureINTELandOpSpecConstantTargetINTELcan be determined from targets registry.