[PATCH] D121554: [mlir][OpenMP] Added translation from `omp.atomic.capture` to LLVM IR

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added inline comments.


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:677
+  let extraClassDeclaration = [{
+    Operation& getFirstOp() {
+      return getRegion().front().getOperations().front();
----------------
Most places use `Operation *`, let's do the same here for consistency.


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:729-733
+    Operation& getFirstOp();
+    Operation& getSecondOp();
+    AtomicReadOp getAtomicReadOp();
+    AtomicWriteOp getAtomicWriteOp();
+    AtomicUpdateOp getAtomicUpdateOp();
----------------
Please add at least some documentation to these methods.


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:734-735
+    AtomicUpdateOp getAtomicUpdateOp();
+    Value getX();
+    Value getV();
+  }];
----------------
It's unclear to be what "x" and "v" mean in this context. Given the trivial bodies of these functions, I would just drop them in favor of `getAtomicReadOp().x()`.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1043
+static LogicalResult
+convertOmpAtomicCapture(omp::AtomicCaptureOp &atomicCaptureOp,
+                        llvm::IRBuilderBase &builder,
----------------
Concrete Ops must be passed by value.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1068-1071
+      return atomicCaptureOp.emitError(
+          "the update operation inside the region must be a binary operation "
+          "and that update operation must have the region argument as an "
+          "operand");
----------------
Please provide a test for user-visible error messages.


================
Comment at: mlir/test/Target/LLVMIR/openmp-llvm.mlir:1030-1031
+  // CHECK: %[[res:.*]] = atomicrmw add i32* %[[x]], i32 %[[expr]] monotonic
+  // CHECK-NEXT: %[[newval:.*]] = add i32 %[[res]], %[[expr]]
+  // CHECK-NEXT: store i32 %[[newval]], i32* %[[v]]
+  omp.atomic.capture {
----------------
Do we really care about this code being on the next line? If not, please drop `-NEXT`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121554