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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse 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,
----------------
Leporacanthicus wrote:
> 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
Why can't that commit be folded into this change? It has not landed yet, and it would be significantly easier to review to bisect if necessary. This may require rebasing some downstream patchset, but that is the usual price to pay for developing downstream.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111051