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

[Mats Petersson via Phabricator via llvm-commits]

Leporacanthicus marked 2 inline comments 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,
----------------
shraiysh wrote:
> 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)?
I've made the simd_modifier into a unit attribute.

This will need to be updated in fir-dev to match, I will make a patch for that when I get this one into llvm/main.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:245
+static ParseResult
+validateModifiers(OpAsmParser &parser,
+                  SmallVectorImpl<SmallString<12>> &modifiers) {
----------------
shraiysh wrote:
> suggestion: renaming it to `verifyScheduleClause` and calling it from `verifyWsLoopOp`?
Rename: I've changed the name to verifyScheduelModifiers to represent what it does.
Calling it from verifyWsLoopOp: some of the error checking done here can't be done at that point - such as checking if there's more than two modifiers, because we no longer have the array of modifiers, only the two that has been stored in the WsLoopOp.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:258
+             (modifiers[0] == "simd" ||
+              (modifiers[1] != "simd" && modifiers[1] != "none")))
+    return parser.emitError(parser.getNameLoc()) << " incorrect modifier order";
----------------
ftynse wrote:
> Nit: please use braces symmterically
Rewritten it in a simpler way to reduce the need for parenthesis.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:269
+/// sched-list ::= sched-val | sched-val sched-list | sched-val `,`
+/// sched-modifier sched-val ::= sched-with-chunk | sched-wo-chunk
 /// sched-with-chunk ::= sched-with-chunk-types (`=` ssa-id-and-type)?
----------------
ftynse wrote:
> Having two terms on the LHS isn't valid BNF AFAIK.
Darned clang-format has messed with my comments! 

I'll fix, but may need to use shorter names.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:274
+/// sched-modifier ::=  sched-modifier-val | sched-modifier-val `,`
+/// sched-modifier-val sched-modifier ::=  `monotonic` | `nonmonotonic` | `simd`
+/// | `none`
----------------
ftynse wrote:
> Ditto, I don't understand what this wants to say.
Ditto clang-format reflowing comments.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:307
       return failure();
+    if (mod != "none" && mod != "monotonic" && mod != "nonmonotonic" &&
+        mod != "simd")
----------------
ftynse wrote:
> ftynse wrote:
> > Nit:  I would have put this into the validation function.
> I believe there is a function `symbolizeScheduleModifier` or something similar that is generated from ODS, which returns `Optional<ScheduleModifier>` equal to `None` if the wrong keyword is used. This avoids magic strings and is also future-proof.
Indeed there is. I'm changing this here, and then doing a similar change for the ScheduleKind which is being tested in form of strings above.


================
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
----------------
shraiysh wrote:
> nit: indentation
Fixing all of the incorrectly formatted tests (all my fault!)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111051