[PATCH] D79937: [MLIR] Support for flush operation, and translating the same to LLVM IR

[Kiran Kumar T P via Phabricator via llvm-commits]

kiranktp marked 15 inline comments as done.
kiranktp added inline comments.


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:321
+  // flush Construct: !$omp flush [(list)]
+  // flush with a list is treated same as a flush without a list by ignoring
+  // the list.
----------------
kiranktp wrote:
> ftynse wrote:
> > This comment doesn't seem to belong here
> This function is specific to openmp Op convertion and also this comment is very generic to flush construct (both C/Fortran). So I placed it here.
I have added the comment around flush construct handling.


================
Comment at: mlir/test/Dialect/OpenMP/ops.mlir:27
+  // Test with one data var
+  // CHECK: omp.flush
+  "omp.flush"(%data_var) : (memref<i32>) -> ()
----------------
ftynse wrote:
> This doesn't actually check that you print back operands
I have updated the test case to validate the operands as well.


================
Comment at: mlir/test/Dialect/OpenMP/ops.mlir:28
+  // CHECK: omp.flush
+  "omp.flush"(%data_var) : (memref<i32>) -> ()
+
----------------
ftynse wrote:
> Could we rather use the custom format?
The current changes looks fine?


================
Comment at: mlir/test/Target/openmp-llvm.mlir:20
+
+  // CHECK: call void @__kmpc_flush(%struct.ident_t* @{{[0-9]+}}
+  omp.flush %arg0, %arg0 : !llvm.i32, !llvm.i32
----------------
kiranchandramohan wrote:
> Is it possible to check that the arguments are also passed correctly for this version of omp.flush? 
Anyway the argument list is being discarded, we need not check for correctness of the arguments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79937