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

Upstream several Intel extensions #176

Merged

Conversation

MrSidims
Copy link
Contributor

@MrSidims MrSidims commented Nov 5, 2020

No description provided.

@MrSidims
Copy link
Contributor Author

MrSidims commented Nov 16, 2020

@bashbaug @mkinsner @johnkslang could you please take a look?

@MrSidims
Copy link
Contributor Author

@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

AlexeySachkov pushed a commit to KhronosGroup/SPIRV-LLVM-Translator that referenced this pull request Dec 16, 2020
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.
@MrSidims MrSidims force-pushed the private/MrSidims/OtherExtensions branch from 9eac30d to 0233c78 Compare December 16, 2020 16:36
vmaksimo pushed a commit to vmaksimo/llvm that referenced this pull request Dec 22, 2020
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.
Copy link
Contributor

@dneto0 dneto0 left a 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",
Copy link
Contributor

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

Copy link
Contributor

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).

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@dneto0
Copy link
Contributor

dneto0 commented Jan 19, 2021

Please rebase this change against master, to keep it fresh.

{ "kind" : "LiteralString", "name" : "'Asm target'" }
],
"capabilities" : [ "AsmINTEL" ],
"extensions" : [ "SPV_INTEL_inline_assembly" ],
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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" ]
Copy link
Contributor

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"

Copy link
Contributor Author

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
Copy link
Contributor

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"

Copy link
Contributor Author

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",
Copy link
Contributor

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"

dneto0 added a commit to dneto0/SPIRV-Tools that referenced this pull request Jan 20, 2021
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
@raunraun
Copy link
Contributor

Will merge when SPIRV-Tiils#4116 is given the OK by the bots.

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>
Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
@MrSidims MrSidims force-pushed the private/MrSidims/OtherExtensions branch from 0233c78 to 0dd29fb Compare January 20, 2021 11:38
@MrSidims
Copy link
Contributor Author

Shall I squash commits before merging to eliminate the last "Apply suggestions" commit?

dneto0 added a commit to KhronosGroup/SPIRV-Tools that referenced this pull request Jan 20, 2021
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
@dneto0
Copy link
Contributor

dneto0 commented Jan 20, 2021

Shall I squash commits before merging to eliminate the last "Apply suggestions" commit?

I don't think it's necessary to squash.

Copy link
Contributor

@dneto0 dneto0 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.
I've checked this with the corresponding SPIRV-Tools change, and the combination works together.
Thanks!

@MrSidims
Copy link
Contributor Author

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!

@johnkslang johnkslang self-requested a review January 20, 2021 16:38
@johnkslang johnkslang merged commit 8bb2420 into KhronosGroup:master Jan 20, 2021
svenvh pushed a commit to svenvh/SPIRV-LLVM-Translator that referenced this pull request Mar 31, 2021
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.
svenvh pushed a commit to KhronosGroup/SPIRV-LLVM-Translator that referenced this pull request Apr 6, 2021
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.
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.

None yet

4 participants