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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

You can avoid the "clang-format not found in user's PATH; not linting file." warning using `arc diff --nolint`. It asks for a justification, it accepts every non-empty string, including a single space. Please consider fixing the other warnings.



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2124-2128
+    /// CurBB
+    /// |     /---\
+		/// ContBB    |
+    /// |     \---/
+    /// ExitBB
----------------
Triple slashes are doxygen comments, which come before the function. If not a doxygen documentation, please use standard `//` comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2129
+    /// ExitBB
+    auto *CurBB = Builder.GetInsertBlock();
+    auto *CurBBTI = CurBB->getTerminator();
----------------
[style] No almost-always-auto in LLVM style


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2161-2164
+    if (Builder.GetInsertPoint() == ContBB->end())
+      llvm::errs() << "end block\n";
+    else
+      ContBB->dump();
----------------
Debugging leftover?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1922
 
+bool OpenMPIRBuilder::CheckAndEmitFlushAfterAtomic(
+    const LocationDescription &Loc, llvm::AtomicOrdering AO, AtomicKind AK) {
----------------
Consider making such helpers static functions instead of priviate methods, so they don't need to appear in the public header file.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1936
+
+  if (AK == Read) {
+    if (AO == AtomicOrdering::Acquire || AO == AtomicOrdering::AcquireRelease ||
----------------
fghanim wrote:
> Meinersbur wrote:
> > This if-chain could be done more elegantly with a switch.
> Either:
> 1- do nested if-else
> 2- nested switch case
> 3- a mix of both.
> 
> 4- use the magic of logical shifts & bit-wise and/or.s to do this, and get rid of all the messy cfg
> 
> switched from 1 to 3, more than happy to do 4
It is fine as-is, just the `Capture` case might justify a nested switch.

Not sure whether approach 4 would make it any better.


================
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:
----------------
fghanim wrote:
> Meinersbur wrote:
> > With a default case, no need to enumerate all cases.
> 
> I intentionally listed all cases. If you want, i'll remove `default` which I added to future proof the code.
With a default case, the compiler does not warn about unhandled cases anymore. If you want both, you could do:

```
  [...]
  case AtomicRMWInst::Min:
    llvm_unreachable("Unsupported atomic update operation");
  }
  llvm_unreacheable("Unhandled atomic update operation");
```
Personally I think it just adds clutter lines. In case of some parts of clang's OpenMP implementation it fills entire screens of unsupported cases and there is no chance I miss handling the case during testing due to the llvm_unreachable.

Currently, `return nullptr;` is unreachable code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99024