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

[Peixin Qiao via Phabricator via llvm-commits]

peixin 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:
> shraiysh wrote:
> > peixin wrote:
> > > shraiysh wrote:
> > > > 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?
> > > However, this is for OpenMP dialect. Currently, OpenMP standard only defines C, C++, and fortran, and all the update statements for them can be transformed as one single instruction and one `omp.yield` operation. `f(x,expr) = (x^expr)^expr` is obviously not allowed in OpenMP standard. If someone want to use this generic atomic update, they should implement one in other dialects in somewhere else, instead of OpenMP dialect here. What you currently implemented is one generic atomic update, not restricted to OpenMP atomic update.
> > > can be transformed as one single instruction and one `omp.yield` operation
> > Yes, in the LLVM IR Dialect. We cannot say the same for all the other dialects with certainty. (or can we? Feel free to correct me if this sounds absurd.) Maybe we can put a check during lowering to LLVM IR for having exactly two operations, but enforcing it in the operation verifier itself is not entirely correct as the internal op can be in any dialect (In some example dialect, a few atomic update statements for some types might need more than a single op in that dialect).
> > > What you currently implemented is one generic atomic update, not restricted to OpenMP atomic update.
> > That is correct, because only the dialect handling the operation knows how to update it appropriately (including how many ops to take for that update).
> I think we can say that. Actually, OpenMP standard says that. Please notice that `OpenMPToLLVMIRTranslation.cpp` is for OpenMP. In semantics, having more than one single op is not correct. If someone needs it, they should not use OpenMP stomic update since it is not allowed in OpenMP atomic update.
OK. I noticed that you changed the omp.atomic.update dialect. https://github.com/llvm/llvm-project/blob/0e38b295435bf49c21e9ff97354a855ac8c78c7e/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td#L637-L638 Here, you referred to OpenMP standard, but you make it too generic so that this opertion is beyond the OpenMP atomic update. I think we need to discuss about the change in D119522. @kiranchandramohan What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119657