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

[Shraiysh via Phabricator via llvm-commits]

shraiysh added inline comments.


================
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) {
----------------
shraiysh wrote:
> 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.
I am sorry, there is an error in the latest rebase too. I do not know why procbind is getting affected by the presence/absence of default value for OMPC_Order. Looking into it.
```
$ ninja check-mlir
undefined reference to `llvm::omp::getProcBindKind(llvm::StringRef)'
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112198