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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse accepted this revision.
ftynse added a comment.
This revision is now accepted and ready to land.

LGTM when all comments are addresesd.



================
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";
----------------
Nit: please use braces symmterically


================
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)?
----------------
Having two terms on the LHS isn't valid BNF AFAIK.


================
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`
----------------
Ditto, I don't understand what this wants to say.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:307
       return failure();
+    if (mod != "none" && mod != "monotonic" && mod != "nonmonotonic" &&
+        mod != "simd")
----------------
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.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:307-310
+    if (mod != "none" && mod != "monotonic" && mod != "nonmonotonic" &&
+        mod != "simd")
+      return parser.emitError(parser.getNameLoc())
+             << " unknown modifier type: " << mod;
----------------
Nit:  I would have put this into the validation function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111051