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

[Shraiysh via Phabricator via llvm-commits]

shraiysh added a comment.

Thanks for this patch. I have a few comments @Leporacanthicus.

Could you please clarify the purpose of `mlir/test/Target/LLVMIR/openmp-llvm-bad-schedule-modifier.mlir`? As far as I can tell, these tests can be a part of `llvm-project/mlir/test/Dialect/OpenMP/invalid.mlir`.



================
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:
> 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.
+1 to SIMD being a unit attribute.

Would it be better to have an ArrayAttr of ScheculeModifier (similar to the spec)?


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:245
+static ParseResult
+validateModifiers(OpAsmParser &parser,
+                  SmallVectorImpl<SmallString<12>> &modifiers) {
----------------
suggestion: renaming it to `verifyScheduleClause` and calling it from `verifyWsLoopOp`?


================
Comment at: mlir/test/Target/LLVMIR/openmp-llvm.mlir:525-526
+  // CHECK: %[[cond:.*]] = icmp ne i32 %[[continue]], 0
+  // CHECK  br i1 %[[cond]], label %omp_loop.header{{.*}}, label %omp_loop.exit{{.*}}
+   llvm.call @body(%iv) : (i64) -> ()
+   omp.yield
----------------
nit: indentation


================
Comment at: mlir/test/Target/LLVMIR/openmp-llvm.mlir:537-538
+  // CHECK: %[[cond:.*]] = icmp ne i32 %[[continue]], 0
+  // CHECK  br i1 %[[cond]], label %omp_loop.header{{.*}}, label %omp_loop.exit{{.*}}
+   llvm.call @body(%iv) : (i64) -> ()
+   omp.yield
----------------
nit: indentation


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111051