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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

There is an RFC <https://llvm.discourse.group/t/rfc-extending-declarative-assembly-format-to-support-order-independent-variadic-segments/4388?u=shraiysh> for better support for unordered lists in MLIR assembly, maybe that could be used. However, as @Leporacanthicus mentioned in the OpenMP/flang conference call, D111051 <https://reviews.llvm.org/D111051> and this are part of upstreaming the fir-dev branch. In the interest of not making the upstreaming process for fir-dev more difficult, we should consider to keep the delta low and defer larger changes after complete upstreaming of fir-dev.



================
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,
----------------
ftynse wrote:
> 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.
Since `simd` is also a schedule modifier, maybe rename `schedule_modifiers`  to `monotonicity_modifier`? I.e. we'd have `monotonicity_[schedule_]modifier` and `simd_[schedule_]modifier`.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:749
+    if (auto modifier = op.schedule_modifiers()) {
+      p << ", " << modifier;
+    }
----------------
ftynse wrote:
> 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.
[serious] I think there might be a real bug here. If no monotonicity modifier is used, but the simd modifier, then according this code will print `schedule(static,simd)`. However, the parsing code will interpret the first modifier as monotonicity modifier, and try to interpret `simd` as one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111051