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

[clang][CodeGen][OpenMP] Fix casting of atomic update of ptr types #88215

Merged
merged 1 commit into from
Apr 12, 2024

Conversation

mikerice1969
Copy link
Contributor

In 4d5e834, casts were removed for pointers but one case was missed. Add missing check.

In 4d5e834, casts were removed for
pointers but one case was missed. Add missing check.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen clang:openmp OpenMP related changes to Clang labels Apr 9, 2024
@mikerice1969
Copy link
Contributor Author

See: https://godbolt.org/z/av7axb9as

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 9, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Mike Rice (mikerice1969)

Changes

In 4d5e834, casts were removed for pointers but one case was missed. Add missing check.


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

2 Files Affected:

  • (modified) clang/lib/CodeGen/CGAtomic.cpp (+5-1)
  • (modified) clang/test/OpenMP/atomic_update_codegen.cpp (+11)
diff --git a/clang/lib/CodeGen/CGAtomic.cpp b/clang/lib/CodeGen/CGAtomic.cpp
index d35ce0409d7232..07452b18a85ea4 100644
--- a/clang/lib/CodeGen/CGAtomic.cpp
+++ b/clang/lib/CodeGen/CGAtomic.cpp
@@ -1806,7 +1806,11 @@ void AtomicInfo::EmitAtomicUpdateOp(
                                              /*NumReservedValues=*/2);
   PHI->addIncoming(OldVal, CurBB);
   Address NewAtomicAddr = CreateTempAlloca();
-  Address NewAtomicIntAddr = castToAtomicIntPointer(NewAtomicAddr);
+  Address NewAtomicIntAddr =
+      shouldCastToInt(NewAtomicAddr.getElementType(), /*CmpXchg=*/true)
+          ? castToAtomicIntPointer(NewAtomicAddr)
+          : NewAtomicAddr;
+
   if ((LVal.isBitField() && BFI.Size != ValueSizeInBits) ||
       requiresMemSetZero(getAtomicAddress().getElementType())) {
     CGF.Builder.CreateStore(PHI, NewAtomicIntAddr);
diff --git a/clang/test/OpenMP/atomic_update_codegen.cpp b/clang/test/OpenMP/atomic_update_codegen.cpp
index ce0765118922a1..fe745590a9f919 100644
--- a/clang/test/OpenMP/atomic_update_codegen.cpp
+++ b/clang/test/OpenMP/atomic_update_codegen.cpp
@@ -27,6 +27,7 @@ long double ldv, ldx;
 _Complex int civ, cix;
 _Complex float cfv, cfx;
 _Complex double cdv, cdx;
+char *cpx;
 
 typedef int int4 __attribute__((__vector_size__(16)));
 int4 int4x;
@@ -851,6 +852,16 @@ int main(void) {
 // CHECK: call{{.*}} @__kmpc_flush(
 #pragma omp atomic seq_cst
   rix = dv / rix;
+
+// CHECK: [[LD_CPX:%.+]] = load atomic ptr, ptr @cpx monotonic
+// CHECK: br label %[[CONT:.+]]
+// CHECK: [[CONT]]
+// CHECK: [[PHI:%.+]] = phi ptr
+// CHECK: [[RES:%.+]] = cmpxchg ptr @cpx,
+// CHECK: br i1 %{{.+}}, label %[[EXIT:.+]], label %[[CONT]]
+  #pragma omp atomic update
+  cpx += 1;
+
   return 0;
 }
 

// CHECK: br label %[[CONT:.+]]
// CHECK: [[CONT]]
// CHECK: [[PHI:%.+]] = phi ptr
// CHECK: [[RES:%.+]] = cmpxchg ptr @cpx,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really end up emitting a cmpxchg in the frontend just because atomicrmw doesn't have pointer typed add? This is horrible

Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

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

We should really fix using cmpxchg here. Can you open an IR issue for it?

@mikerice1969
Copy link
Contributor Author

We should really fix using cmpxchg here. Can you open an IR issue for it?

Sure, I'll look into it and follow-up. Thanks for the review!

@mikerice1969 mikerice1969 merged commit 3652b2a into llvm:main Apr 12, 2024
8 checks passed
@mikerice1969 mikerice1969 deleted the atomic-ptr-fix branch April 12, 2024 17:01
bazuzi pushed a commit to bazuzi/llvm-project that referenced this pull request Apr 15, 2024
…lvm#88215)

In 4d5e834, casts were removed for
pointers but one case was missed. Add missing check.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen clang:openmp OpenMP related changes to Clang clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants