[PATCH] D79410: [MLIR] [OpenMP] Add basic OpenMP parallel operation

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added inline comments.


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:67
+  let arguments = (ins Optional<I1>:$if_expr_var,
+             Optional<AnySignedInteger>:$num_threads_var,
+             OptionalAttr<ClauseDefault>:$default_val,
----------------
kiranchandramohan wrote:
> ftynse wrote:
> > ftynse wrote:
> > > Do we care about integer being signed for num_threads?
> > Could you please elaborate why you need a _signed_ integer here, and not just `AnyInteger`. And what is the semantics of passing a negative value here.
> > 
> > Generally, given the number of attributes to this op, it really needs a `description` field.
> 1) I think it is OK to be AnyInteger. Is your point that if we only allow it to be unsigned then a conversion is needed to create this operation if the value of num_integer is stored in an unsigned variable? 
> I am providing some relevant information, please advise whether AnyInteger is the most suitable.
> 
> If a negative value is passed, it should be an error. But the standard allows for it to be an expression which can evaluate to a positive value.
> 
> The OpenMP standard has the following specification and restriction for num_threads.
> num_threads(scalar-integer-expression)
> The num_threads expression must evaluate to a positive integer value.
> 
> The related omp_set_num_threads OpenMP API call in C takes in a signed integer
> void omp_set_num_threads(int num_threads);
> 
> 2) The other related point is that the frontend might have evaluated this to a value already. If that is always the case then num_threads can be an attribute. But I am assuming that the mlir operation should be generic enough to allow what is described in the standard (scalar-integer-expression which evaluates to a positive value).
I think involving the signed/unsigned/signless difference is adding unnecessary complexity here. This is not fixing the bitwidth of the integer either... Also, most operations in the standard dialect, which are most likely to be used to implement the expression, operate on signless integers so extra work will be necessary to pass the expression result to an operation that only accepts signed integers.

MLIR lets you declare a constant value via `std.constant`. So if a frontend has folded `num_threads` into a constant, it can just emit one more operation and use the `omp.parallel` version expecting a value.

Please add a `description` field to this operation and let's get this landed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79410