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

[AMDGPU] Remove unnecessary conditionality on atomic CSUB pseudo-ops #69914

Merged
merged 1 commit into from Oct 23, 2023

Conversation

stepthomas
Copy link
Contributor

The pseudo-ops for BUFFER_ATOMIC_CSUB and GLOBAL_ATOMIC_CSUB should not
be conditional on FeatureAtomicCSubNoRtnInsts, as that feature rightly
should only control whether or not the non-returning forms are available
for instruction selection, not whether they exist or not.

Change-Id: Icb2fcdcf6c7bc55e48b522d0eec8dd35637c768e

The pseudo-ops for BUFFER_ATOMIC_CSUB and GLOBAL_ATOMIC_CSUB should not
be conditional on FeatureAtomicCSubNoRtnInsts, as that feature rightly
should only control whether or not the non-returning forms are available
for instruction selection, not whether they exist or not.

Change-Id: Icb2fcdcf6c7bc55e48b522d0eec8dd35637c768e
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 23, 2023

@llvm/pr-subscribers-backend-amdgpu

Author: Stephen Thomas (stepthomas)

Changes

The pseudo-ops for BUFFER_ATOMIC_CSUB and GLOBAL_ATOMIC_CSUB should not
be conditional on FeatureAtomicCSubNoRtnInsts, as that feature rightly
should only control whether or not the non-returning forms are available
for instruction selection, not whether they exist or not.

Change-Id: Icb2fcdcf6c7bc55e48b522d0eec8dd35637c768e


Full diff: https://github.com/llvm/llvm-project/pull/69914.diff

2 Files Affected:

  • (modified) llvm/lib/Target/AMDGPU/BUFInstructions.td (+1-6)
  • (modified) llvm/lib/Target/AMDGPU/FLATInstructions.td (+1-5)
diff --git a/llvm/lib/Target/AMDGPU/BUFInstructions.td b/llvm/lib/Target/AMDGPU/BUFInstructions.td
index e1bdf0e3bc1bf29..897bbfa5c58bee2 100644
--- a/llvm/lib/Target/AMDGPU/BUFInstructions.td
+++ b/llvm/lib/Target/AMDGPU/BUFInstructions.td
@@ -1047,14 +1047,9 @@ defm BUFFER_ATOMIC_DEC_X2 : MUBUF_Pseudo_Atomics <
 >;
 
 let SubtargetPredicate = HasGFX10_BEncoding in {
-  defm BUFFER_ATOMIC_CSUB : MUBUF_Pseudo_Atomics_RTN <
+  defm BUFFER_ATOMIC_CSUB : MUBUF_Pseudo_Atomics <
     "buffer_atomic_csub", VGPR_32, i32, int_amdgcn_global_atomic_csub
   >;
-
-  let OtherPredicates = [HasAtomicCSubNoRtnInsts] in
-    defm BUFFER_ATOMIC_CSUB : MUBUF_Pseudo_Atomics_NO_RTN <
-      "buffer_atomic_csub", VGPR_32, i32
-    >;
 }
 
 let SubtargetPredicate = isGFX8GFX9 in {
diff --git a/llvm/lib/Target/AMDGPU/FLATInstructions.td b/llvm/lib/Target/AMDGPU/FLATInstructions.td
index 533013a3130c05f..52c1c6230cd5541 100644
--- a/llvm/lib/Target/AMDGPU/FLATInstructions.td
+++ b/llvm/lib/Target/AMDGPU/FLATInstructions.td
@@ -871,12 +871,8 @@ defm GLOBAL_ATOMIC_DEC_X2 : FLAT_Global_Atomic_Pseudo <"global_atomic_dec_x2",
                               VReg_64, i64>;
 
 let SubtargetPredicate = HasGFX10_BEncoding in {
-  defm GLOBAL_ATOMIC_CSUB : FLAT_Global_Atomic_Pseudo_RTN <"global_atomic_csub",
+  defm GLOBAL_ATOMIC_CSUB : FLAT_Global_Atomic_Pseudo <"global_atomic_csub",
                                 VGPR_32, i32>;
-
-  let OtherPredicates = [HasAtomicCSubNoRtnInsts] in
-    defm GLOBAL_ATOMIC_CSUB : FLAT_Global_Atomic_Pseudo_NO_RTN <"global_atomic_csub",
-                                  VGPR_32, i32>;
 }
 
 defm GLOBAL_LOAD_LDS_UBYTE  : FLAT_Global_Load_LDS_Pseudo <"global_load_lds_ubyte">;

Copy link
Contributor

@jayfoad jayfoad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK to me, though I don't understand why setting OtherPredicates on the Pseudo did not get copied to the corresponding Real instructions and prevent them from being assembled/disassembled.

@stepthomas stepthomas merged commit 2326b2b into llvm:main Oct 23, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants