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

[Shraiysh via Phabricator via llvm-commits]

shraiysh added inline comments.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:961
+
+/// Converts an OpenMP atomic update operation using OpenMPIRBuilder.
+static LogicalResult
----------------
peixin wrote:
> 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.
Sure. Added in the description for `omp.atomic.update` operation in OpenMPOps.td.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:980
+
+  if (innerUpdateOp.getNumOperands() == 2 &&
+      llvm::is_contained(innerUpdateOp.getOperands(),
----------------
peixin wrote:
> 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?
Yes, thank you. Added.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:983
+                         opInst.getRegion().getArgument(0)))
+    binop = convertBinOpToAtomic(innerUpdateOp);
+
----------------
peixin wrote:
> 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?
I have added a TODO in `OMPIRBuilder.cpp` for this. It goes into error at the moment. This is a bug in `OmpIRBuilder`.
```
  // TODO: handle the case where XElemTy is not byte-sized or not a power of 2.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119657