-
Notifications
You must be signed in to change notification settings - Fork 266
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
Upstream several Intel extensions #176
Upstream several Intel extensions #176
Conversation
01f0fd1
to
fb44893
Compare
@bashbaug @mkinsner @johnkslang could you please take a look? |
@mkinsner @bashbaug could you please take a look and possibly merge? It looks like misalignment between SPIR-V headers starts bringing and attention: intel/llvm#2889 |
New spirv.hpp is generated from github.com/KhronosGroup/SPIRV-Headers using following PRs: - KhronosGroup/SPIRV-Headers#176 - KhronosGroup/SPIRV-Headers#177 with an exception of `SPV_INTEL_fpga_loop_fuse` and `SPV_INTEL_long_constant_composite` extensions definitions that were added manually in this PR.
9eac30d
to
0233c78
Compare
New spirv.hpp is generated from github.com/KhronosGroup/SPIRV-Headers using following PRs: - KhronosGroup/SPIRV-Headers#176 - KhronosGroup/SPIRV-Headers#177 with an exception of `SPV_INTEL_fpga_loop_fuse` and `SPV_INTEL_long_constant_composite` extensions definitions that were added manually in this PR.
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.
So far this change looks ok.
For one thing, I verified that all the enum values are placed in the right order.
I'll need to prepare a patch for SPIRV-Tools to support the new enumerant kinds FPDenorm_Mode and FPOperation_Mode
@@ -9088,6 +9333,34 @@ | |||
} | |||
] | |||
}, | |||
{ | |||
"category" : "ValueEnum", | |||
"kind" : "FPDenormMode", |
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.
Adding a new operand kind like this will break the SPIRV-Tools build.
I plan to prepare a patch to SPIRV-Tools to absorb this change.
This is the kind of error I get when rebasing this change against master and rebuilding SPIRV-Tools:
In file included from ...spirv-tools/source/operand.cpp:36:
third_party/spirv-tools/operand.kinds-unified1.inc:703:172: error: ‘SPV_OPERAND_TYPE_FPDENORM_MODE’ was not declared in this scope; did you mean ‘SPV_OPERAND_TYPE_MEMORY_MODEL’?
703 | {"FunctionDenormModeINTEL", 5823, 1, pygen_variable_caps_FunctionFloatControlINTEL, 1, pygen_variable_exts_SPV_INTEL_float_controls2, {SPV_OPERAND_TYPE_LITERAL_INTEGER, SPV_OPERAND_TYPE_FPDENORM_MODE}, 0xffffffffu, 0xffffffffu},
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| SPV_OPERAND_TYPE_MEMORY_MODEL
third_party/spirv-tools/operand.kinds-unified1.inc:724:179: error: ‘SPV_OPERAND_TYPE_FPOPERATION_MODE’ was not declared in this scope; did you mean ‘SPV_OPERAND_TYPE_EXECUTION_MODE’?
724 | {"FunctionFloatingPointModeINTEL", 6080, 1, pygen_variable_caps_FunctionFloatControlINTEL, 1, pygen_variable_exts_SPV_INTEL_float_controls2, {SPV_OPERAND_TYPE_LITERAL_INTEGER, SPV_OPERAND_TYPE_FPOPERATION_MODE}, 0xffffffffu, 0xffffffffu},
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| SPV_OPERAND_TYPE_EXECUTION_MODE
third_party/spirv-tools/operand.kinds-unified1.inc:1229:4: error: ‘SPV_OPERAND_TYPE_FPDENORM_MODE’ was not declared in this scope; did you mean ‘SPV_OPERAND_TYPE_MEMORY_MODEL’?
1229 | {SPV_OPERAND_TYPE_FPDENORM_MODE, ARRAY_SIZE(pygen_variable_FPDenormModeEntries), pygen_variable_FPDenormModeEntries},
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| SPV_OPERAND_TYPE_MEMORY_MODEL
third_party/spirv-tools/operand.kinds-unified1.inc:1230:4: error: ‘SPV_OPERAND_TYPE_FPOPERATION_MODE’ was not declared in this scope; did you mean ‘SPV_OPERAND_TYPE_EXECUTION_MODE’?
1230 | {SPV_OPERAND_TYPE_FPOPERATION_MODE, ARRAY_SIZE(pygen_variable_FPOperationModeEntries), pygen_variable_FPOperationModeEntries},
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| SPV_OPERAND_TYPE_EXECUTION_MODE
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 have a pending patch for SPIRV-Tools. KhronosGroup/SPIRV-Tools#4116
That will have to land before this patch (to prevent the SPIRV-Tools build from breaking).
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.
Yeah, it also required changes in the headers builder itself.
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 landed the SPIRV-Tools change, so we're clear to land this one, from my perspective.
Please rebase this change against master, to keep it fresh. |
{ "kind" : "LiteralString", "name" : "'Asm target'" } | ||
], | ||
"capabilities" : [ "AsmINTEL" ], | ||
"extensions" : [ "SPV_INTEL_inline_assembly" ], |
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 guidance is that the new capability enums have an "extensions" list. In turn, the instructions and non-capability enumerants are guarded by a new capability. I find it more clear that way.
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.
Just to clarify, the suggestion is to remove extensions
list from codes' definitions, right?
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.
Yes, for everything that isn't a capability.
The capability should keep it.
"enumerant" : "AllowContractFastINTEL", | ||
"value" : "0x10000", | ||
"capabilities" : [ "FPFastMathModeINTEL" ], | ||
"extensions" : [ "SPV_INTEL_fp_fast_math_mode" ] |
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 this and the next entry should have version set to "None"
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.
Thanks! Missed that one.
"enumerants" : [ | ||
{ | ||
"enumerant" : "Preserve", | ||
"value" : 0 |
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 it is preferable to add:
"capabilities" : [ "FunctionFloatControlINTEL" ],
"version" : "None"
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.
Thanks! It wasn't mentioned in the spec, but I guess you are right, I'll open a spec issue.
"kind" : "FPOperationMode", | ||
"enumerants" : [ | ||
{ | ||
"enumerant" : "IEEE", |
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.
Similarly, I think it's preferable to add:
"capabilities" : [ "FunctionFloatControlINTEL" ],
"version" : "None"
This patch supports new Intel extensions added via KhronosGroup/SPIRV-Headers#176 SPV_INTEL_fooat_controls2 requires extra support to add two new operand types: SPV_OPERAND_TYPE_FPDENORM_MODE SPV_OPERAND_TYPE_FPOPERATION_MODE
Will merge when SPIRV-Tiils#4116 is given the OK by the bots. |
Spec: https://github.com/intel/llvm/blob/2237b42035f31cb10b16d4f9abaeed45bed98587/sycl/doc/extensions/SPIRV/SPV_INTEL_fpga_buffer_location.asciidoc Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Spec: https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/SPIRV/SPV_INTEL_inline_assembly.asciidoc Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Spec: https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/SPIRV/SPV_INTEL_arbitrary_precision_int.asciidoc Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Spec: https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/SPIRV/SPV_INTEL_usm_storage_classes.asciidoc Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Spec: https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/SPIRV/SPV_INTEL_variable_length_array.asciidoc Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Spec: https://github.com/KhronosGroup/SPIRV-Registry/blob/master/extensions/INTEL/SPV_INTEL_io_pipes.asciidoc Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Spec: https://github.com/KhronosGroup/SPIRV-Registry/blob/master/extensions/INTEL/SPV_INTEL_fpga_memory_accesses.asciidoc Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Spec: https://github.com/intel/llvm/blob/e185a6b49e4bc9806a799b774977f1196b24f0d6/sycl/doc/extensions/SPIRV/SPV_INTEL_vector_compute.asciidoc Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Spec: https://github.com/intel/llvm/blob/39fa9b0cbfbae88327118990a05c5b387b56d2ef/sycl/doc/extensions/SPIRV/SPV_INTEL_float_controls2.asciidoc Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Spec: https://github.com/KhronosGroup/SPIRV-Registry/blob/7d96a31cf56c60de76a6ae7a26ace3c7bfd999bf/extensions/INTEL/SPV_INTEL_fpga_cluster_attributes.asciidoc https://github.com/KhronosGroup/SPIRV-Registry/blob/7d96a31cf56c60de76a6ae7a26ace3c7bfd999bf/extensions/INTEL/SPV_INTEL_fp_fast_math_mode.asciidoc Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
0233c78
to
0dd29fb
Compare
Shall I squash commits before merging to eliminate the last "Apply suggestions" commit? |
This patch supports new Intel extensions added via KhronosGroup/SPIRV-Headers#176 SPV_INTEL_fooat_controls2 requires extra support to add two new operand types: SPV_OPERAND_TYPE_FPDENORM_MODE SPV_OPERAND_TYPE_FPOPERATION_MODE
I don't think it's necessary to squash. |
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.
Looks good to me.
I've checked this with the corresponding SPIRV-Tools change, and the combination works together.
Thanks!
Good news! Thanks for making the things going! |
New spirv.hpp is generated from github.com/KhronosGroup/SPIRV-Headers using following PRs: - KhronosGroup/SPIRV-Headers#176 - KhronosGroup/SPIRV-Headers#177 with an exception of `SPV_INTEL_fpga_loop_fuse` and `SPV_INTEL_long_constant_composite` extensions definitions that were added manually in this PR.
New spirv.hpp is generated from github.com/KhronosGroup/SPIRV-Headers using following PRs: - KhronosGroup/SPIRV-Headers#176 - KhronosGroup/SPIRV-Headers#177 with an exception of `SPV_INTEL_fpga_loop_fuse` and `SPV_INTEL_long_constant_composite` extensions definitions that were added manually in this PR.
No description provided.