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

[Shraiysh via Phabricator via llvm-commits]

shraiysh added inline comments.


================
Comment at: mlir/test/Target/LLVMIR/openmp-llvm.mlir:921
+  ^bb0(%xval: i32):
+    %t1 = llvm.mul %xval, %expr : i32
+    %t2 = llvm.sdiv %t1, %expr : i32
----------------
peixin wrote:
> peixin wrote:
> > I don't think you should process the lowering of the "expr" in MLIR stage. For different programing languages, the expr may have different structure. Can you try to analyze the "expr" when lowering parse-tree to MLIR? That is, the input for the "expr" in atomic update is always one mlir value and the LLVM IR generated for analyzing the "expr" is outside the atomic update region. Another reason for doing this is that "expr" cannot access the storage location designated by x, and the operations inside the atomic update region will be very clean. That is, you will always only have two instructions in atomic update region, one for update "xval", the other one for yield the "newval".
> Similar to https://github.com/flang-compiler/f18-llvm-project/blob/9e96e8e4b1f84db0c30eaf4798f094f571b1c565/flang/lib/Lower/OpenMP.cpp#L320-L322. Using genExprValue should provide you the result value of the "expr".
The lowering of `%expr` is not handled in the MLIR region. It is assumed to be a single lowered value, with lowering handled in Flang as you suggested. The idea behind allowing more than one operations was to have the following in MLIR
```
omp.atomic.update %x {
^bb0(%xval):
  %xnew = f(%xval, %expr)
  omp.yield %xnew
}
```
when we cannot express `x = f(x, expr)` in MLIR with a single instruction (for example `f(x,expr) = (x^expr)^expr)`) as we might not have the appropriate single op in the dialect (which can be anything including but not limited to LLVM IR Dialect and FIR Dialect) and that should be okay. In this case, we cannot have `f(x, expr)` such that the operation can be done in one op. By allowing multiple operations in the region, we provide the liberty of handling this in multiple ops. The generated LLVM IR will work appropriately. Another thing is that the MLIR Operation cannot depend on Flang, because this operation will be used by multiple dialects, so it has to be generic enough to support simple and complicated Dialects. Should I put a comment about this in the code itself?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119657