-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[SPIRV] Support for SPV_INTEL_cluster_attributes extension #131593
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
Draft
EbinJose2002
wants to merge
3
commits into
llvm:main
Choose a base branch
from
EbinJose2002:cluster_attributes
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
22 changes: 22 additions & 0 deletions
22
llvm/test/CodeGen/SPIRV/extensions/SPV_INTEL_cluster_attributes/cluster_attributes.ll
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| ; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown --spirv-ext=+SPV_INTEL_fpga_cluster_attributes %s -o - | FileCheck %s | ||
|
|
||
| ; CHECK-DAG: OpCapability FPGAClusterAttributesINTEL | ||
| ; CHECK-DAG: OpCapability FPGAClusterAttributesV2INTEL | ||
| ; CHECK-DAG: OpExtension "SPV_INTEL_fpga_cluster_attributes" | ||
| ; CHECK-DAG: OpDecorate %[[#STALLENABLE_DEC:]] StallEnableINTEL | ||
| ; CHECK-DAG: OpDecorate %[[#STALLFREE_DEC:]] StallFreeINTEL | ||
| ; CHECK: %[[#STALLENABLE_DEC]] = OpFunction %[[#]] None %[[#]] | ||
| ; CHECK: %[[#STALLFREE_DEC]] = OpFunction %[[#]] None %[[#]] | ||
|
|
||
| define spir_func void @test_fpga_stallenable_attr() !stall_enable !0 { | ||
| entry: | ||
| ret void | ||
| } | ||
|
|
||
| define spir_func void @test_fpga_stallfree_attr() !stall_free !1 { | ||
| entry: | ||
| ret void | ||
| } | ||
|
|
||
| !0 = !{ i32 1 } | ||
| !1 = !{ i32 1 } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Lets not use internal to https://github.com/intel/llvm metadata. Instead lets use spirv.Decorations metadata defined here: https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/main/docs/SPIRVRepresentationInLLVM.rst (note, SPIR-V friendly LLVM IR is used not only in the translator, but also in SPIR-V backend).
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 for the reply. I understand that the use of Intel-specific metadata like stall_enable is discouraged, and the recommendation is to use the !spirv.Decorations metadata.
But I had a few questions:
Should the !spirv.Decorations metadata be expected to already exist in the LLVM IR (i.e., injected by the frontend or via an earlier pass), or am I expected to modify the frontend to emit this?
Alternatively, is it acceptable for me to assume that !spirv.Decorations will be present in the .ll file (e.g., from manually written tests or generated via a pass), and implement the behavior based on its presence?
Uh oh!
There was an error while loading. Please reload this page.
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.
As I have previously stated in #134352 (comment) - I don't really understand the end goal to give a precise and educated suggestion. So this question should be readdressed to your internship curator, what are you going to do with these extensions (I'm judging that there is one from your reply: "Actually I was implementing this extension as part of an internship project."). My point is: that it's better not to use internal to intel/llvm metadata. If the goal is just to modify SPIR-V backend that way, that it can emit SPV_INTEL_cluster_attributes - spirv.Decorations metadata is the way to go. Otherwise - if there are further plans to use these extensions, then it's still better to expect spirv.Decorations in the backend (and target it by the frontend) unless proven otherwise.
Also, I can't tell for sure as I haven't tested it, but my expectation is that nowadays llvm-spirv translator supports SPV_INTEL_cluster_attributes via spirv.Decorations metadata implicitly.
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.
Actually there is no particular use for this extension for us as of now. This was part of being familiar with llvm-project repo. So I tried to implement the missing extensions the same way they are handled in the translator. (Also the translator is using "stall_enable" and "stall_free" metadata to handle the extension)
So should I proceed with implementing the extension by modifying the frontend?