[MLIR, OpenMP] Introduce extra helper function for clauses of `ParallelOp`

[SouraVX]

Summary

These functions can be leveraged while synthesizing attibutes. A typical use case for this is lowering to OpenMPDialect in Flang.

[SouraVX]

If we agreed upon this, the next step would be to remodel default clause as UnitAttr also.

[rriddle]

What kind of issues? This seems like a regression in terms of usability. Can you describe in detail the problems that you were having?

[SouraVX]

In D89340#2328518, @rriddle wrote:

What kind of issues? This seems like a regression in terms of usability. Can you describe in detail the problems that you were having?

Thanks for review!
Agreed with your perspective.
The problem I was faced was that, ParseTree models these clauses as enum. Can we synthesize an Attribute using enum value ?
Furthermore, this clause though initially modelled as OptionalAttr, fits well as a UnitAttr. Does changing them now has deeper implications that I'm unaware of ?

[kiranchandramohan]

@SouraVX You can check functions for creating enum/string values for proc_bind in the following files in the build directory.
./tools/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsEnums.h.inc
./tools/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsEnums.cpp.inc

Once you have the string I guess you can create the StringAttr easily.

[SouraVX]

Ah, potential oversight of details(Sorry for that!). Anyway I've rolled back to initial version and introduced some extra declaration that can be used by Flang(once this patch is in) while lowering.
Aside from this, Should we promote/remodel these attributes as UnitAttr? What do you guys think?

[kiranchandramohan]

I guess @clementval or @rriddle can approve the new extra functions added based on its need in Flang parse-tree lowering or on similar usage for StrEnumAttributes in rest of MLIR.

Aside from this, Should we promote/remodel these attributes as UnitAttr? What do you guys think?

I feel the current modelling as an Optional StrEnumAttribute is fine here. proc_bind and default takes a set of values which are best captured by this modelling. If we have these as unit attributes then all the values can occur at the same time unless disabled using the verifier.

[SouraVX]

In D89340#2331031, @kiranchandramohan wrote:

I guess @clementval or @rriddle can approve the new extra functions added based on its need in Flang parse-tree lowering or on similar usage for StrEnumAttributes in rest of MLIR.

Snippet of usability from an upcoming/in-progress patch.

 switch (procBindClause->v) {
+                     case Fortran::parser::OmpProcBindClause::Type::Master:
+                     parallelOp.proc_bind_valAttr(firOpBuilder.getStringAttr(parallelOp.getProcBindValueMasterAttrName()));
+                             break;
+                     case Fortran::parser::OmpProcBindClause::Type::Close:
+                     parallelOp.proc_bind_valAttr(firOpBuilder.getStringAttr(parallelOp.getProcBindValueCloseAttrName()));
+                             break;
+                     case Fortran::parser::OmpProcBindClause::Type::Spread:
+                     parallelOp.proc_bind_valAttr(firOpBuilder.getStringAttr(parallelOp.getProcBindValueSpreadAttrName()));
+                             break;

Aside from this, Should we promote/remodel these attributes as UnitAttr? What do you guys think?

I feel the current modelling as an Optional StrEnumAttribute is fine here. proc_bind and default takes a set of values which are best captured by this modelling. If we have these as unit attributes then all the values can occur at the same time unless disabled using the verifier.

+1

[SouraVX]

Ping! @ all reviewers.

[kiranchandramohan]

File ./tools/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsEnums.cpp.inc has the following function. Isn't it possible to use this function?
For e.g: If you pass it mlir::omp::ClauseProcBindKind::master it will return the string "master".

::llvm::StringRef stringifyClauseProcBindKind(ClauseProcBindKind val) {

switch (val) {
  case ClauseProcBindKind::master: return "master";
  case ClauseProcBindKind::close: return "close";
  case ClauseProcBindKind::spread: return "spread";
}
return "";

}

[SouraVX]

Thanks @kiranchandramohan for suggestion. I already tried this thing(didn't worked out).
Unless I'm missing something: There's a type mismatch while interfacing this function. Here's the snippet of usability

case Fortran::parser::OmpProcBindClause::Type::Master:
          parallelOp.proc_bind_valAttr(firOpBuilder.getStringAttr(
           omp::stringifyClauseProcBindKind(procBindClause->v)));
          break;

Error:

error: cannot convert ‘const Fortran::parser::OmpProcBindClause::Type’ to ‘mlir::omp::ClauseProcBindKind’ for argument ‘1’ to ‘llvm::StringRef mlir::omp::stringifyClauseProcBindKind(mlir::omp::ClauseProcBindKind)’
              omp::stringifyClauseProcBindKind(procBindClause->v)));

An alternative solution would be: (but it's hard coding) ? (do you vote for this?)

case Fortran::parser::OmpProcBindClause::Type::Master:
          parallelOp.proc_bind_valAttr(firOpBuilder.getStringAttr(
           StringRef("master")));
          break;

[kiranchandramohan]

1. Does the EnumToString function in the parser work for this enum value?

2. Otherwise, hardcoding is necessary. The solution you are proposing (below) also has hardcoding isn't it?

switch (procBindClause->v) {
+ case Fortran::parser::OmpProcBindClause::Type::Master:
+ parallelOp.proc_bind_valAttr(firOpBuilder.getStringAttr(parallelOp.getProcBindValueMasterAttrName()));

I was suggesting to use the following

switch (procBindClause->v) {
                     case Fortran::parser::OmpProcBindClause::Type::Master:
                     parallelOp.proc_bind_valAttr(firOpBuilder.getStringAttr(omp::stringifyClauseProcBindKind(omp::ClauseProcBindKind::master)));

[tskeith]

In D89340#2334906, @kiranchandramohan wrote:

1. Does the EnumToString function in the parser work for this enum value?

EnumToString should work on any enum class declared using ENUM_CLASS. That's the best way to avoid repeating the name in a string.

[SouraVX]

Thanks @kiranchandramohan for helping out in this :). I'm abandoning this in favor of your approach. (Updating the PR also)
Thanks @tskeith for suggestion, I'll take a note of this.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

let arguments = (ins Optional<AnyType>:$if_expr_var,
              Variadic<AnyType>:$shared_vars,
              Variadic<AnyType>:$copyin_vars,
              OptionalAttr<ProcBindKind>:$proc_bind_val);
 
   let regions = (region AnyRegion:$region);
 
   let parser = [{ return parseParallelOp(parser, result); }];
   let printer = [{ return printParallelOp(p, *this); }];
+  let extraClassDeclaration = [{
+    static StringRef getDefaultValuePrivateAttrName() { return "defprivate"; }
+    static StringRef getDefaultValueFirstPrivateAttrName() { return "deffirstprivate"; }
+    static StringRef getDefaultValueSharedAttrName() { return "defshared"; }
+    static StringRef getDefaultValueNoneAttrName() { return "defnone"; }
+    static StringRef getProcBindValueMasterAttrName() { return "master"; }
+    static StringRef getProcBindValueCloseAttrName() { return "close"; }
+    static StringRef getProcBindValueSpreadAttrName() { return "spread"; }
+  }];
 }
 
 def TerminatorOp : OpenMP_Op<"terminator", [Terminator]> {
   let summary = "terminator for OpenMP regions";
   let description = [{
     A terminator operation for regions that appear in the body of OpenMP
     operation.  These regions are not expected to return any value so the
     terminator takes no operands. The terminator op returns control to the