[PATCH] D86071: [MLIR][OpenMP] Add omp.do operation

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula requested changes to this revision.
bondhugula added a comment.
Herald added a subscriber: tatianashp.

Great to see this. Some minor comments.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMP.td:120
+
+// static and auto are C++ keywords so need a capital to disambiguate
+def OMP_SCHEDULE_Static : ClauseVal<"Static", 2, 1> {}
----------------
Please terminate with a period.


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:134
+    associate loops are executed in. Currently the only option for this
+    attribute is "concurrent".
+  }];
----------------
This description is missing a customary example in the end. (Please use triple backticks to include an example.)


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:144
+             Optional<AnyType>:$schedule_chunk_var,
+             OptionalAttr<AnyI64Attr>:$collapse_val,
+             OptionalAttr<UnitAttr>:$nowait,
----------------
Please `Confined` and an `IntMinValue` argument to force this to be positive. It'll be automatically enforced and verified. As an example, please see `AllocLikeOp` in the standard dialect.

```
 let arguments = (ins Variadic<Index>:$value,
                   Confined<OptionalAttr<I64Attr>, [IntMinValue<0>]>:$alignment);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86071