[PATCH] D111051: [mlir][OpenMP] Add support for SIMD modifier

[Alex Zinenko via Phabricator via llvm-commits]

ftynse requested changes to this revision.
ftynse added inline comments.
This revision now requires changes to proceed.


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:230-231
              Optional<AnyType>:$schedule_chunk_var,
+             OptionalAttr<ScheduleModifier>:$schedule_modifiers,
+             OptionalAttr<ScheduleModifier>:$simd_modifier,
              Confined<OptionalAttr<I64Attr>, [IntMinValue<0>]>:$collapse_val,
----------------
I'm not sure I understand the modeling here. The same enum attribute is listed twice with different names, and there is no verification of what can go where. It looks like one can have `OMP_SCHEUDLE_MOD_SIMD` as `schedule_modifiers` and `OMP__SCHEDULE_MOD_None` as `simd_modifier`. Or even monotonic in one and non-monotonic in the other.

Should SIMD be just a unit attribute?

Also consider renaming `schedule_modifiers` to `schedule_modifier` (singular) because it can only contain a single instance anyway.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:413
 /// sched-with-chunk-types ::= `static` | `dynamic` | `guided`
 /// sched-wo-chunk ::=  `auto` | `runtime`
 static ParseResult
----------------
Nit: you may need to update this comment


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:688
+        result.addAttribute("simd_modifier", mod);
+      }
+    }
----------------
This needs to error out with a message if there is more than two modifiers.

We also need a verifier that checks that modifiers are what we expect them to be, e.g., there's no conflict between monotonic and non-monotonic, no repetition, etc. See the remark above.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:749
+    if (auto modifier = op.schedule_modifiers()) {
+      p << ", " << modifier;
+    }
----------------
It looks preferable to have the default case here (modifier is `none` and missing modifier are the same, aren't they?) and just avoid extra printing in that case altogether.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111051