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

[Shraiysh via Phabricator via llvm-commits]

shraiysh added a comment.

Thanks for the questions @kiranchandramohan ! I have tried to address them. Please let me know what you think.



================
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
----------------
kiranchandramohan wrote:
> 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?
We can change this error message for invalid clauses in general, but changing it specifically for "inclusive" might not be simple with the current implementation. AFAIU, we have two choices - 
 - Make inclusive a clause and change the syntax back to original, just for OpenMP Dialect (might be useful for other loop-related pragmas too).
 - Change the error message to include something like "attribute might be placed wrongly".



================
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) {
----------------
kiranchandramohan wrote:
> 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?
This error is not seen in the latest rebase. So we can use it without the default value now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112198