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

[Kiran Chandramohan via Phabricator via llvm-commits]

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

Thanks. LGTM.



================
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:
> > 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".
> 
OK. No need to change now. We can later handle this appropriately as an attribute in general syntax, using open-closed interval syntax etc.


================
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:
> 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)'
> ```
There is an error before that which leads to the OMP*.inc files not being generated and possibly the first missing function is getProcBindKind which causes an undefined reference.


```
[3/41] Building OMP.inc...
error: At least one val in Clause order must be defined as default.
```

Looking at this again I think this is OK, we do not generate documentation for the OrderKind values. And default does not seem to be in OrderKind.

```
def OrderKindconcurrent : StrEnumAttrCase<"concurrent">;
def OrderKind: StrEnumAttr<
  "ClauseOrderKind",
  "OrderKind Clause",
  [OrderKindconcurrent]> {
    let cppNamespace = "::mlir::omp";
}
```

BTW, proc_bind uses the unknown value for this purpose. Would it be better to reuse this value (unknown) instead of default?
def OMP_PROC_BIND_unknown : ClauseVal<"unknown",7,0> { let isDefault = true; }



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112198