[PATCH] D112198: [MLIR][OpenMP] Fixed the missing inclusive clause in omp.wsloop and fix order clause

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

I have a couple of questions.



================
Comment at: mlir/test/Dialect/OpenMP/invalid.mlir:150
+  // expected-error @below {{inclusive is not a valid clause}}
+  omp.wsloop (%iv) : index = (%lb) to (%ub) step (%step) nowait inclusive {
+    omp.yield
----------------
shraiysh wrote:
> kiranchandramohan wrote:
> > Is this an error because inclusive is not placed after ub?
> Yes, as you suggested, `inclusive` is not a clause and so, it is expected outside the clause-list. I placed it right after the upper bound because it only affects the upper bound.
Since we have not documented the difference between regular OpenMP clauses and OpenMP dialect attributes/operands, it might be confusing to get this error. Would it be possible to give the error regarding the placement?


================
Comment at: mlir/test/Dialect/OpenMP/invalid.mlir:158
+func @order_value(%lb : index, %ub : index, %step : index) {
+  // expected-error @below {{attribute 'order_val' failed to satisfy constraint: OrderKind Clause}}
+  omp.wsloop (%iv) : index = (%lb) to (%ub) step (%step) order(default) {
----------------
peixin wrote:
> shraiysh wrote:
> > peixin wrote:
> > > shraiysh wrote:
> > > > kiranchandramohan wrote:
> > > > > Why is this an error?
> > > > This is an error because `order(default)` is not allowed to be specified by the user according to the standard. Order can either be present with the `concurrent` value, or it should not be there.
> > > It seems that order clause can only be present with the `concurrent` value. Where does the `OMP_ORDER_default` in OMP.td come from? I don't see this in OpenMP 5.0 Spec.
> > Yes, that is correct. It is not in the OpenMP Spec, but as far as I understand, every Clause in OMP.td is supposed to have a default value (which signifies no value, I believe). I faced a similar issue when dealing with the `memory_order` clause. There was no `default` memory clause in the standard, and not adding a default value was leading to some errors in either one of llvm/clang/flang (I don't exactly remember which one). @clementval please correct me if I have missed something.
> Thanks for the explanation. This is fine with me.
I think ClauseVal is currently only used for generating string attributes for some clauses in the OpenMP MLIR dialect. This change will list default as a possible option but the verifier will not permit it which I believe is not an ideal situation.
https://mlir.llvm.org/docs/Dialects/OpenMPDialect/#attributes-4

Can we do something else here?
1) Since only one value is allowed can this be changed to a unit attribute?
2) Check whether the requirement for default can be relaxed? Where did you see this error?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112198