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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added inline comments.
Herald added a subscriber: jurahul.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79410