[SYCL] Extend global offset intrinsic removal #11909
Merged
Conversation
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
added 7 commits
November 14, 2023 17:43
Extend intel#11674 by modifying the globaloffset optimization pass to always replace uses of Loads from the llvm.nvvm.implicit.offset and llvm.amdgcn.implicit.offset intrinsics with constant zeros in the original non-offset kernel. Hence, perform the optimization even when -enable-global-offset=true (default). Duplicate recursively functions containing calls to the implicit offset intrinsic and let the implicit offset kernel entry point only call the original functions (i.e. do not call the functions with added offset arguments). Remove zero allocations for the original kernel entry points.
Use std::get to retrieve an element from a tuple that used to be pair previously. Do not mention deleted allocas of zeros anymore.
Fix an incomplete removal of calls to the implicit offset intrinsics. The global-offset-multiple-calls-from-one-function.ll test highlighted this flaw inside the IR. Recurse only non cloned functions for adding offset parameters and obtain call intructions that are to be replaced through a global VMap.
Check for functions with and without offset parameters inside the NVPTX and AMDGPU global-offset test cases. Take into account that functions that contain calls to the global-offset intrinsic or transitively call one of these functions have two different versions now - one with and one without offset argument. Check that the correct ones are called. Add also a new test case for mixed functions, i.e. functions containing direct calls to the intrinsic and functions that eventually transitively call the intrinsic. Do this to check if the recursion inside the global offset pass handles these correctly.
Copy the global offset correctly to addrspace(3) inside the kernel. Do not perform this operation if the cloned function is not a kernel entry point. Adapt the AMDGPU test cases to take the copying of the global offset into account
MartinWehking
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
MartinWehking
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
MartinWehking
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
MartinWehking
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
Before the global-offset optimization, non-offset and offset kernels called the same function with an added offset argument. The non-offset versions allocated an array with 3 zeros in addrspace(5) and passed it as an argument to the function calls containing calls to the intrinsic. Since the allocation and filling is not possible in addrspace(4), the offset version adds an extra alloca in addrspace(5) and does an addrspace cast to addrspace(4). This is not required anymore since the non-offset version does not perform an alloca of zeros anymore. However, the optimization is rendered almost useless as the global offset feature is deprecated. Adapt the comment to reflect this.
MartinWehking
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
MartinWehking
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
MartinWehking
changed the title
MartinWehking
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
MartinWehking
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
jchlanda
approved these changes
Dec 6, 2023
| continue; | ||
|
|
||
| // Kernel entry points need additional processing and change Metdadata. | ||
| if (EntryPointMetadata.count(Caller) != 0) |
There was a problem hiding this comment.
I don't know if it's a good idea to change it. There are two reasons:
- We're dealing here with natural numbers (counts), not bools
- I didn't introduce the change originally, so it would add more diff
There was a problem hiding this comment.
EntryPointMetadata is a map and therefore .count() is often used as contains, i.e. implicit conversion to bool should be ok in that context.
But I agree with (2)
llvm/test/CodeGen/AMDGPU/global-offset-intrinsic-function-mix.ll
Outdated
Show resolved
Hide resolved
MartinWehking
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
MartinWehking
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
AlexeySachkov
approved these changes
Dec 7, 2023
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.
Extend #11674 by modifying the globaloffset optimization pass to always
replace uses of Loads from the
llvm.nvvm.implicit.offsetandllvm.amdgcn.implicit.offset intrinsicswith constant zeros in theoriginal non-offset kernel.
Hence, perform the optimization even when
-enable-global-offset=true(default).Duplicate recursively functions containing calls to the implicit offset
intrinsic and let the implicit offset kernel entry point only call
the original functions (i.e. do not call the functions with added offset
arguments).
Remove zero allocations for the original kernel entry points.