[PATCH] D99024: [OpenMP][OMPIRBuilder] Adding support for `omp atomic`

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Could you please address the numerous clang-format/clang-tidy warnings?



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1925-1926
+
+  if (AO == AtomicOrdering::NotAtomic || AO == llvm::AtomicOrdering::Unordered)
+    llvm_unreachable("Unexpected ordering.");
+
----------------



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1933-1934
+
+  // todo: when the above happens uncomment this & all relevant lines below.
+  // llvm::AtomicOrdering FlushAO;
+
----------------
Please remove all code in comments; The TODO should explain what there is todo.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1936
+
+  if (AK == Read) {
+    if (AO == AtomicOrdering::Acquire || AO == AtomicOrdering::AcquireRelease ||
----------------
This if-chain could be done more elegantly with a switch.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1973
+                                  AtomicOrdering AO) {
+
+  if (!updateToLocation(Loc))
----------------
[nit] Unnecessary newline


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1977
+
+  auto XTy = X.Var->getType();
+  assert(XTy->isPointerTy() && "OMP Atomic expects a pointer to target memory");
----------------
[style] [[ https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable | The LLVM source style does not use almost always auto ]].


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1992
+  } else {
+    // we need to bitcast and perform atomic op as integer
+    unsigned addrspace = cast<PointerType>(XTy)->getAddressSpace();
----------------
[style] Please start sentences with a capital letter.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2082-2088
+  case AtomicRMWInst::Xchg:
+  case AtomicRMWInst::FAdd:
+  case AtomicRMWInst::FSub:
+  case AtomicRMWInst::BAD_BINOP:
+  case AtomicRMWInst::Max:
+  case AtomicRMWInst::Min:
+  default:
----------------
With a default case, no need to enumerate all cases.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99024/new/

https://reviews.llvm.org/D99024