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

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

kiranktp added inline comments.


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:320
   }
+  // flush Construct: !$omp flush [(list)]
+  // flush with a list is treated same as a flush without a list by ignoring
----------------
ftynse wrote:
> Let's not use Fortran syntax in C++ code
I will remove the Fortran syntax


================
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.
----------------
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.


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:322
+  // flush with a list is treated same as a flush without a list by ignoring
+  // the list.
   return llvm::TypeSwitch<Operation *, LogicalResult>(&opInst)
----------------
kiranchandramohan wrote:
> Is this a TODO?
> Do we require another version of CreateFlush in the OpenMP Builder to support the list?
Openmp runtime will discard the list even if its passed. This is the same behavior with clang and legacy flang as well. The list will be discarded for flush construct. 
Same has been documented in the OpenMP standard as well. 
>From OpenMP 4.5 Standard Doc:
"An implementation may implement a flush with a list by ignoring the list, and treating it the same as a flush without a list."
"Note – Use of a flush construct with a list is extremely error prone and users are strongly discouraged from attempting it."
I think this is the reason, there is only one version of CreateFlush without this list.
If the decision is to process the list and pass it on to IR builder, another version of CreateFlush has to be implemented.
In spite of this, the list will be discarded and a call to __kmpc_flush will be inserted without the list in IR builder.


================
Comment at: mlir/test/Target/openmp-llvm.mlir:4
+// CHECK-LABEL: define void @empty(i32 %0)
+llvm.func @empty(%arg0: !llvm.i32) {
   // CHECK: [[OMP_THREAD:%.*]] = call i32 @__kmpc_global_thread_num(%struct.ident_t* @{{[0-9]+}})
----------------
kiranchandramohan wrote:
> Shall we have more functions to test? Maybe one for those with arguments and one for those without? Also, rename this function.
I will add a separate function with arguments and give a meaning full name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79937