[SYCL][FPGA] Rename [[intel::disable_loop_pipelining]] attribute function metadata #11372
Conversation
This file contains 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
…data
Metadata emitted from front-end when an FPGA IPA kernel has the property pipelined<N> is a function metadata of the form !disable_loop_pipelining !X, !X = !{i32 0|1}.
This is confusing as the user means to pipeline the kernel/not pipeline the kernel as opposed to enabling/disabling the pipelining of all loops in the kernel/function, and this name does not reflect that. This patch renames the functin metadata to !disable_kernel_pipelining.
This was likely caused by a miscommunication with the related loop metadata that's currently being attached when the loop attribute [[intel::disable_loop_pipelining]] is used: {!"llvm.loop.intel.pipelining.enable", i32 0}
This patch also changes the loop metadata to !llvm.loop.intel.pipelining.disable and invert the argument to reflet the correct attribute behavior and match with function metadata change.
Signed-off-by: Soumi Manna <soumi.manna@intel.com>
smanna12
changed the title
Signed-off-by: Soumi Manna <soumi.manna@intel.com>
smanna12
commented
Oct 4, 2023
| indicates that the kernel should be pipelined or not. Cannot be used on the | ||
| same loop or function, or in conjunction with ``speculated_iterations``, | ||
| ``max_concurrency``, ``initiation_interval``, ``ivdep``, | ||
| ``max_reinvocation_delay`` or ``enable_loop_pipelining`` attribute. |
There was a problem hiding this comment.
Thanks @bowenxue-intel for reviews. We added support for "enable_loop_pipeling" on #9263
|
@smanna12 Thanks for the response, I also just noticed that there is the enable_loop_pipelining attribute. In this case, I am double-checking whether it makes sense for the loop metadata emitted when [[intel::disable_loop_pipelining]] is used to be changed, or if it can remain as is, which would result in less work on the SPIR-V and our middle-end. Other than that, LGTM |
bowenxue-intel
approved these changes
Oct 4, 2023
Signed-off-by: Soumi Manna <soumi.manna@intel.com>
Signed-off-by: Soumi Manna <soumi.manna@intel.com>
smanna12
changed the title
Signed-off-by: Soumi Manna <soumi.manna@intel.com>
|
@bowenxue-intel, could you please review updated patch? Thank you |
bowenxue-intel
approved these changes
Oct 6, 2023
|
Thank you @bowenxue-intel for reviews! Related SPIRV PR: KhronosGroup/SPIRV-LLVM-Translator#2171 @intel/dpcpp-cfe-reviewers, please review. Thank you |
MrSidims
pushed a commit
to KhronosGroup/SPIRV-LLVM-Translator
that referenced
this pull request
Oct 9, 2023
…or loops and functions (#2171) Refactor SPIRVWriter to accept !disable_kernel_pipelining instead of !disable_loop_pipelining for function metadata, and !llvm.loop.intel.pipelining.disable instead of !llvm.loop.intel.pipelining.enable for loop metadata. Refactor SPIRVReader to emit !disable_kernel_pipelining instead of !disable_loop_pipelining for function metadata, and !llvm.loop.intel.pipelining.disable instead of !llvm.loop.intel.pipelining.enable for loop metadata. Relevant/Related intel/llvm PR: intel/llvm#11372
|
@elizabethandrews / @premanandrao could you please review? Thank you |
elizabethandrews
approved these changes
Oct 10, 2023
jsji
pushed a commit
that referenced
this pull request
Oct 12, 2023
…or loops and functions (#2171) Refactor SPIRVWriter to accept !disable_kernel_pipelining instead of !disable_loop_pipelining for function metadata, and !llvm.loop.intel.pipelining.disable instead of !llvm.loop.intel.pipelining.enable for loop metadata. Refactor SPIRVReader to emit !disable_kernel_pipelining instead of !disable_loop_pipelining for function metadata, and !llvm.loop.intel.pipelining.disable instead of !llvm.loop.intel.pipelining.enable for loop metadata. Relevant/Related intel/llvm PR: #11372 Original commit: KhronosGroup/SPIRV-LLVM-Translator@e68ddf7
Signed-off-by: Soumi Manna <soumi.manna@intel.com>
smanna12
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
smanna12
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
Latest commit 624a580 was just a small update in document. @intel/llvm-gatekeepers, This PR is ready to be merged. Thank you |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Metadata emitted from front-end when an FPGA IPA kernel has the property pipelined is a function metadata of the form !disable_loop_pipelining !X, !X = !{i32 0|1}.
This is confusing as the user means to pipeline the kernel/not pipeline the kernel as opposed to enabling/disabling the pipelining of all loops in the kernel/function, and this name does not reflect that. This patch renames the function metadata to "!pipeline_kernel", and to flip the argument. We believe it may convey information better about what to do with the kernel when the property is absent.
This was likely caused by a miscommunication with the related loop metadata that's currently being attached when the loop attribute [[intel::disable_loop_pipelining]] is used: {!"llvm.loop.intel.pipelining.enable", i32 0}