[PATCH] D119657: [OpenMP][mlir] Lowering for omp.atomic.update

[Alex Zinenko via Phabricator via llvm-commits]

ftynse requested changes to this revision.
ftynse added inline comments.
This revision now requires changes to proceed.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:978-979
+      .Case([&](LLVM::XOrOp) { return llvm::AtomicRMWInst::BinOp::Xor; })
+      // .Case([&](LLVM::MaxNumOp) { return llvm::AtomicRMWInst::BinOp::Max; })
+      // .Case([&](LLVM::MinNumOp) { return llvm::AtomicRMWInst::BinOp::Min; })
+      .Case([&](LLVM::UMaxOp) { return llvm::AtomicRMWInst::BinOp::UMax; })
----------------
Please do not commit commented-out code.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:992
+                       LLVM::ModuleTranslation &moduleTranslation) {
+  // Boilerplate.
+  llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
----------------
This comment is not helpful, we can see it is boilerplate. I would rather have most of it factored out into a function like `getOpenMPLoc(IRBuilderBase &)`.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1002
+  // Convert values and types.
+  Operation &updateOp = *opInst.region().op_begin();
+  bool isXBinopExpr =
----------------
There doesn't seem to be a check for the region to only have two operations (binop and terminator), and this translation does not account for it but just ignores the remaining operations.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1004
+  bool isXBinopExpr =
+      updateOp.getOperand(0) == opInst.getRegion().getArgument(0);
+  mlir::Value mlirExpr =
----------------
There doesn't seem to be any check that `updateOp` does have operands at all anywhere.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1013
+      moduleTranslation.convertType(xtype.getElementType());
+  llvm::OpenMPIRBuilder::AtomicOpValue X = {x, xElementType, /*isSigned=*/false,
+                                            /*isVolatile=*/false};
----------------
Please use better names than `x` and `X` in the same function, these are extremely hard to keep in one's head for more than a couple of lines.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1017
+  llvm::AtomicRMWInst::BinOp binop = convertBinOpToAtomic(updateOp);
+  llvm::AtomicOrdering ao = convertAtomicOrdering(opInst.memory_order());
+
----------------
Please consider a better name than `ao`.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1022
+  auto updateFn = [&](llvm::Value *atomicx,
+                      llvm::IRBuilder<> &IRB) -> llvm::Value * {
+    Block &bb = *opInst.region().begin();
----------------
MLIR uses `camelBack` names, `IRB` isn't something that it would use.


================
Comment at: mlir/test/Target/LLVMIR/openmp-llvm.mlir:829
 
+// Checking simple atomicrmw and cmpxchg based lowerings. This also checks for
+// ambigous alloca insert point by putting llvm.mul as the first update operation.
----------------
Note that MLIR tends to use "lowering" for _conversions_ between dialects rather than for _translations_ between MLIR and other IRs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119657