[SYCL][HIP] Add AMDGPU reflect pass to choose between safe and unsafe AMDGPU atomics #11467
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
hdelan
commented
Oct 9, 2023
hdelan
changed the title
ldrumm
reviewed
Oct 9, 2023
ldrumm
reviewed
Oct 9, 2023
ldrumm
requested changes
Oct 9, 2023
The func will remain in the module and a linking error will result at some stage. Which is the same behaviour as the LLVM reflect pass |
hdelan
force-pushed
the
hip-use-unsafe-atomics-flag
branch
from
379d3a6
to
e5657aa
Compare
hdelan
force-pushed
the
hip-use-unsafe-atomics-flag
branch
from
e5657aa
to
b4617ef
Compare
hdelan
force-pushed
the
hip-use-unsafe-atomics-flag
branch
from
b4617ef
to
57ae613
Compare
hdelan
force-pushed
the
hip-use-unsafe-atomics-flag
branch
3 times, most recently
from
5a8c9ac
to
9051df1
Compare
hdelan
force-pushed
the
hip-use-unsafe-atomics-flag
branch
2 times, most recently
from
1872497
to
bad104b
Compare
hdelan
force-pushed
the
hip-use-unsafe-atomics-flag
branch
from
0eda761
to
982ea7a
Compare
hdelan
force-pushed
the
hip-use-unsafe-atomics-flag
branch
from
982ea7a
to
4d17f4f
Compare
hdelan
force-pushed
the
hip-use-unsafe-atomics-flag
branch
from
df15e57
to
7f2771b
Compare
- Change getNumOperands to arg size - Move size check above the for loop
|
@frasercrmck all your comments have been addressed. Thanks for review! In terms of function vs module pass - I understand that it might be slightly more optimal to make this a module pass, however I also think for comprehensibility this pass should not diverge too much from NVVMReflect, which is a func pass. Let me know if you think that this is OK |
|
Friendly ping @intel/dpcpp-tools-reviewers |
ldrumm
approved these changes
Apr 23, 2024
- Use a vector of CallInsts instead of Instructions. - Change assert(fasle) to report_fatal_error.
|
Thanks @ldrumm changes made |
hdelan
force-pushed
the
hip-use-unsafe-atomics-flag
branch
from
6c5ef4e
to
c4ab1ed
Compare
AlexeySachkov
approved these changes
Apr 24, 2024
frasercrmck
reviewed
Apr 24, 2024
- Use auto - Use drop_back to remove null byte - Replace hip_be with hip
Change opt test to use update_test_checks.py
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.
AMDGPU reflect pass is needed to choose between safe and unsafe atomics
at the libclc level. In the long run we will delete this patch as work
is being done to ensure correct lowering of atomic instructions. See
patches:
llvm/llvm-project#85052
llvm/llvm-project#69229
This work is necessary as malloc shared atomics rely on PCIe atomics
which can have patchy and unreliable support. Therefore, we want to be
able to choose at compile time whether we should use safe atomics using
CAS (which PCIe should support), or if we want to rely of the
availability of the newest PCIe atomics, if malloc shared atomics are
desired.
Also changes the implementation of
atomic_or,atomic_andso that theycan choose between the safe or unsafe version based on the AMDGPU
reflect value.