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

[Mats Petersson via Phabricator via llvm-commits]

Leporacanthicus marked an inline comment as done.
Leporacanthicus added inline comments.


================
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,
----------------
Meinersbur wrote:
> 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`.
I haven't completely fixed this, but i have added error checking code in a separate commit [to keep changes here to a minimum -> helping the rebase on fir-dev to avoid too much problems]. https://reviews.llvm.org/D112526


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:413
 /// sched-with-chunk-types ::= `static` | `dynamic` | `guided`
 /// sched-wo-chunk ::=  `auto` | `runtime`
 static ParseResult
----------------
ftynse wrote:
> Nit: you may need to update this comment
Done as a separate commit. https://reviews.llvm.org/D112526


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:688
+        result.addAttribute("simd_modifier", mod);
+      }
+    }
----------------
ftynse wrote:
> 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.
Done in a separate commit:
https://reviews.llvm.org/D112526


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:749
+    if (auto modifier = op.schedule_modifiers()) {
+      p << ", " << modifier;
+    }
----------------
Meinersbur wrote:
> 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.
Done in a separate commit, see https://reviews.llvm.org/D112526


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111051