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

[Peixin Qiao via Phabricator via llvm-commits]

peixin added a comment.

The lowering for memory-order-clause and hint clause will be in future work, right?



================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:961
+
+/// Converts an OpenMP atomic update operation using OpenMPIRBuilder.
+static LogicalResult
----------------
Can you add some comments to describe your design method to tell others why you don't do it similar to atomic read/write? You have expalined this in slack. Can you move those info here? Also, comment that the input is checked in semantic analysis for each programming language.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:980
+
+  if (innerUpdateOp.getNumOperands() == 2 &&
+      llvm::is_contained(innerUpdateOp.getOperands(),
----------------
Should this be one if condition or one assert? If I understand correctly, the input not satisfying the if condition is not one standard OpenMP atomic update statement. Right?


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:983
+                         opInst.getRegion().getArgument(0)))
+    binop = convertBinOpToAtomic(innerUpdateOp);
+
----------------
Will this return BAD_BINOP for int128, fp128, x86_fp80? They seems not to be supported in AtomicRMWInst. Can you add one test case for one of them to represent the corner case?

Also, what about the complex data type?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119657