[PATCH] D77634: [MLIR] Support for taskwait and taskyield operations, and translating the same to LLVM IR

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

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


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:56
+  let parser = [{ return success(); }];
+  let printer = [{ p << getOperationName(); }];
+}
----------------
mehdi_amini wrote:
> The declarative assembly syntax may be shorter here :)
Did you mean, this declarative syntax is too short here. Could you please elaborate a bit on this?


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:292-293
 
+/// Given a single OpenMP MLIR operation, create the corresponding LLVM IR
+/// operation.
+LogicalResult
----------------
kiranchandramohan wrote:
> There might be more than one LLVM IR operation corresponding to an OpenMP MLIR operation.
> 
> Would the following be better?
> Given an OpenMP MLIR operation, create the corresponding LLVM IR (including OpenMP runtime calls).
But this function will be called for single OpenMP MLIR operation.
Nevertheless, your wording for comment is neutral and more clear. I will incorporate them.


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:313
+    return success();
+  }
+  return opInst.emitError("unsupported OpenMP operation: ") << opInst.getName();
----------------
mehdi_amini wrote:
> The sequence may be representable with a TypeSwitch here?
I was not aware of TypeSwitch feature. Thanks for the suggestion Mehdi. I will modify as you suggested.


================
Comment at: mlir/test/Target/openmp-llvm.mlir:6-9
+// CHECK: [[OMP_THREAD1:%.*]] = call i32 @__kmpc_global_thread_num(%struct.ident_t* @{{[0-9]+}})
+// CHECK-NEXT:  [[RET_VAL:%.*]] = call i32 @__kmpc_omp_taskwait(%struct.ident_t* @{{[0-9]+}}, i32 [[OMP_THREAD1]])
+// CHECK: [[OMP_THREAD2:%.*]] = call i32 @__kmpc_global_thread_num(%struct.ident_t* @{{[0-9]+}})
+// CHECK-NEXT:  [[RET_VAL:%.*]] = call i32 @__kmpc_omp_taskyield(%struct.ident_t* @{{[0-9]+}}, i32 [[OMP_THREAD2]], i32 0)
----------------
SouraVX wrote:
> kiranchandramohan wrote:
> > Should we move all these checks closer to the operation? What is the MLIR/LLVM style for this?
> They can be moved closer to operations https://llvm.org/docs/CommandGuide/FileCheck.html, it's also better for better readibility and cleanly extending test case further in future.
Thanks for the review Kiran.
Yes, we can move these checks closure to the operation. I will do this modification.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77634