[mlir][OpenMP] Generating enums in accordance with the guidelines

[shraiysh]

Summary

This patch changes the enums generated from OMP.td for MLIR according
to the enum naming guidelines in LLVM Coding Standards.

This also helps the issues we had with static being a C++ keyword and
also a value for the schedule clause.

Enumerator naming guidelines: https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

[shraiysh]

Ping! Rebase with main.

[kiranchandramohan]

LGTM. Please add a pointer to the the llvm guidelines recommending this change.

Does OpenACC use any of this infra currently?

llvm/unittests/Frontend/OpenMPParsingTest.cpp

76–77

Nit: Can this FIXME be removed?

mlir/tools/mlir-tblgen/DirectiveCommonGen.cpp

73–77

Nit: Is the const significant here?

[shraiysh]

Thanks for the review. Addressed comments.

[shraiysh]

Does OpenACC use any of this infra currently?

The tests pass so I don't think they use this. I can see they use ClauseVal once here but the generated enum is not used anywhere so I think we are safe.

[shraiysh]

Removed another comment which was no longer needed.

flang/lib/Lower/OpenMP.cpp

if (blockDirective.v == llvm::omp::OMPD_parallel) {
     for (const auto &clause : parallelOpClauseList.v) {
       // TODO: Handle default clause
       if (const auto &procBindClause =
               std::get_if<Fortran::parser::OmpClause::ProcBind>(&clause.u)) {
         const auto &ompProcBindClause{procBindClause->v};
         omp::ClauseProcBindKind pbKind;
         switch (ompProcBindClause.v) {
         case Fortran::parser::OmpProcBindClause::Type::Master:
-          pbKind = omp::ClauseProcBindKind::master;
+          pbKind = omp::ClauseProcBindKind::Master;
           break;
         case Fortran::parser::OmpProcBindClause::Type::Close:
-          pbKind = omp::ClauseProcBindKind::close;
+          pbKind = omp::ClauseProcBindKind::Close;
           break;
         case Fortran::parser::OmpProcBindClause::Type::Spread:
-          pbKind = omp::ClauseProcBindKind::spread;
+          pbKind = omp::ClauseProcBindKind::Spread;
           break;
         }
         parallelOp.proc_bind_valAttr(omp::ClauseProcBindKindAttr::get(
             firOpBuilder.getContext(), pbKind));
       }
     }
     createBodyOfOp<omp::ParallelOp>(parallelOp, firOpBuilder, currentLocation);
   } else if (blockDirective.v == llvm::omp::OMPD_master) {

llvm/include/llvm/Frontend/OpenMP/OMP.td

let allowedClauseValues = [
     OMP_PROC_BIND_close,
     OMP_PROC_BIND_spread,
     OMP_PROC_BIND_default,
     OMP_PROC_BIND_unknown
   ];
 }
 
 // static and auto are C++ keywords so need a capital to disambiguate.
-def OMP_SCHEDULE_Static : ClauseVal<"Static", 2, 1> {}
-def OMP_SCHEDULE_Dynamic : ClauseVal<"Dynamic", 3, 1> {}
-def OMP_SCHEDULE_Guided : ClauseVal<"Guided", 4, 1> {}
-def OMP_SCHEDULE_Auto : ClauseVal<"Auto", 5, 1> {}
-def OMP_SCHEDULE_Runtime : ClauseVal<"Runtime", 6, 1> {}
-def OMP_SCHEDULE_Default : ClauseVal<"Default", 7, 0> { let isDefault = 1; }
+def OMP_SCHEDULE_Static : ClauseVal<"static", 2, 1> {}
+def OMP_SCHEDULE_Dynamic : ClauseVal<"dynamic", 3, 1> {}
+def OMP_SCHEDULE_Guided : ClauseVal<"guided", 4, 1> {}
+def OMP_SCHEDULE_Auto : ClauseVal<"auto", 5, 1> {}
+def OMP_SCHEDULE_Runtime : ClauseVal<"runtime", 6, 1> {}
+def OMP_SCHEDULE_Default : ClauseVal<"default", 7, 0> { let isDefault = 1; }
 
 def OMPC_Schedule : Clause<"schedule"> {
   let clangClass = "OMPScheduleClause";
   let flangClass = "OmpScheduleClause";
   let enumClauseValue = "ScheduleKind";
   let allowedClauseValues = [
     OMP_SCHEDULE_Static,
     OMP_SCHEDULE_Dynamic,

llvm/unittests/Frontend/OpenMPParsingTest.cpp

TEST(OpenMPParsingTest, getProcBindKind) {
   EXPECT_EQ(getProcBindKind("spread"), OMP_PROC_BIND_spread);
   EXPECT_EQ(getProcBindKind("default"), OMP_PROC_BIND_default);
   EXPECT_EQ(getProcBindKind("unknown"), OMP_PROC_BIND_unknown);
 }
 
 TEST(OpenMPParsingTest, getScheduleKind) {
   EXPECT_EQ(getScheduleKind("foobar"), OMP_SCHEDULE_Default);
 
   // FIXME: Why are these not lower case?
-  EXPECT_EQ(getScheduleKind("Static"), OMP_SCHEDULE_Static);
-  EXPECT_EQ(getScheduleKind("Dynamic"), OMP_SCHEDULE_Dynamic);
-  EXPECT_EQ(getScheduleKind("Guided"), OMP_SCHEDULE_Guided);
-  EXPECT_EQ(getScheduleKind("Auto"), OMP_SCHEDULE_Auto);
-  EXPECT_EQ(getScheduleKind("Runtime"), OMP_SCHEDULE_Runtime);
-  EXPECT_EQ(getScheduleKind("Default"), OMP_SCHEDULE_Default);
+  EXPECT_EQ(getScheduleKind("static"), OMP_SCHEDULE_Static);
+  EXPECT_EQ(getScheduleKind("dynamic"), OMP_SCHEDULE_Dynamic);
+  EXPECT_EQ(getScheduleKind("guided"), OMP_SCHEDULE_Guided);
+  EXPECT_EQ(getScheduleKind("auto"), OMP_SCHEDULE_Auto);
+  EXPECT_EQ(getScheduleKind("runtime"), OMP_SCHEDULE_Runtime);
+  EXPECT_EQ(getScheduleKind("default"), OMP_SCHEDULE_Default);
 }
 
 } // namespace

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp

if (failed(parser.resolveOperands(linears, linearTypes, linears[0].location,
         failed(parser.resolveOperands(linearSteps, linearStepTypes,
                                       linearSteps[0].location,
                                       result.operands)))
       return failure();
   }
 
   // Add schedule parameters
   if (done[scheduleClause] && !schedule.empty()) {
-    schedule[0] = llvm::toUpper(schedule[0]);
     if (Optional<ClauseScheduleKind> sched =
             symbolizeClauseScheduleKind(schedule)) {
       auto attr = ClauseScheduleKindAttr::get(parser.getContext(), *sched);
       result.addAttribute("schedule_val", attr);
     } else {
       return parser.emitError(parser.getCurrentLocation(),
                               "invalid schedule kind");
     }

 }
 
 //===----------------------------------------------------------------------===//
 // Verifier for AtomicReadOp
 //===----------------------------------------------------------------------===//
 
 LogicalResult AtomicReadOp::verify() {
   if (auto mo = memory_order_val()) {
-    if (*mo == ClauseMemoryOrderKind::acq_rel ||
-        *mo == ClauseMemoryOrderKind::release) {
+    if (*mo == ClauseMemoryOrderKind::Acq_rel ||
+        *mo == ClauseMemoryOrderKind::Release) {
       return emitError(
           "memory-order must not be acq_rel or release for atomic reads");
     }
   }
   if (x() == v())
     return emitError(
         "read and write must not be to the same location for atomic reads");
   return verifySynchronizationHint(*this, hint_val());
 }
 
 //===----------------------------------------------------------------------===//
 // Verifier for AtomicWriteOp
 //===----------------------------------------------------------------------===//
 
 LogicalResult AtomicWriteOp::verify() {
   if (auto mo = memory_order_val()) {
-    if (*mo == ClauseMemoryOrderKind::acq_rel ||
-        *mo == ClauseMemoryOrderKind::acquire) {
+    if (*mo == ClauseMemoryOrderKind::Acq_rel ||
+        *mo == ClauseMemoryOrderKind::Acquire) {
       return emitError(
           "memory-order must not be acq_rel or acquire for atomic writes");
     }
   }
   return verifySynchronizationHint(*this, hint_val());
 }
 
 //===----------------------------------------------------------------------===//
 // Verifier for AtomicUpdateOp
 //===----------------------------------------------------------------------===//
 
 LogicalResult AtomicUpdateOp::verify() {
   if (auto mo = memory_order_val()) {
-    if (*mo == ClauseMemoryOrderKind::acq_rel ||
-        *mo == ClauseMemoryOrderKind::acquire) {
+    if (*mo == ClauseMemoryOrderKind::Acq_rel ||
+        *mo == ClauseMemoryOrderKind::Acquire) {
       return emitError(
           "memory-order must not be acq_rel or acquire for atomic updates");
     }
   }
 
   if (region().getNumArguments() != 1)
     return emitError("the region must accept exactly one argument");
 

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

static void convertOmpOpRegions(
   // be converted several times, that is cloned, without clashes, and slightly
   // speeds up the lookups.
   moduleTranslation.forgetMapping(region);
 }
 
 /// Convert ProcBindKind from MLIR-generated enum to LLVM enum.
 static llvm::omp::ProcBindKind getProcBindKind(omp::ClauseProcBindKind kind) {
   switch (kind) {
-  case omp::ClauseProcBindKind::close:
+  case omp::ClauseProcBindKind::Close:
     return llvm::omp::ProcBindKind::OMP_PROC_BIND_close;
-  case omp::ClauseProcBindKind::master:
+  case omp::ClauseProcBindKind::Master:
     return llvm::omp::ProcBindKind::OMP_PROC_BIND_master;
-  case omp::ClauseProcBindKind::primary:
+  case omp::ClauseProcBindKind::Primary:
     return llvm::omp::ProcBindKind::OMP_PROC_BIND_primary;
-  case omp::ClauseProcBindKind::spread:
+  case omp::ClauseProcBindKind::Spread:
     return llvm::omp::ProcBindKind::OMP_PROC_BIND_spread;
   }
   llvm_unreachable("Unknown ClauseProcBindKind kind");
 }
 
 /// Converts the OpenMP parallel operation to LLVM IR.
 static LogicalResult
 convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,

 
 /// Convert an Atomic Ordering attribute to llvm::AtomicOrdering.
 llvm::AtomicOrdering
 convertAtomicOrdering(Optional<omp::ClauseMemoryOrderKind> ao) {
   if (!ao)
     return llvm::AtomicOrdering::Monotonic; // Default Memory Ordering
 
   switch (*ao) {
-  case omp::ClauseMemoryOrderKind::seq_cst:
+  case omp::ClauseMemoryOrderKind::Seq_cst:
     return llvm::AtomicOrdering::SequentiallyConsistent;
-  case omp::ClauseMemoryOrderKind::acq_rel:
+  case omp::ClauseMemoryOrderKind::Acq_rel:
     return llvm::AtomicOrdering::AcquireRelease;
-  case omp::ClauseMemoryOrderKind::acquire:
+  case omp::ClauseMemoryOrderKind::Acquire:
     return llvm::AtomicOrdering::Acquire;
-  case omp::ClauseMemoryOrderKind::release:
+  case omp::ClauseMemoryOrderKind::Release:
     return llvm::AtomicOrdering::Release;
-  case omp::ClauseMemoryOrderKind::relaxed:
+  case omp::ClauseMemoryOrderKind::Relaxed:
     return llvm::AtomicOrdering::Monotonic;
   }
   llvm_unreachable("Unknown ClauseMemoryOrderKind kind");
 }
 
 /// Convert omp.atomic.read operation to LLVM IR.
 static LogicalResult
 convertOmpAtomicRead(Operation &opInst, llvm::IRBuilderBase &builder,

mlir/test/Dialect/OpenMP/ops.mlir

^bb0(%iv: index):
       omp.yield
   }) {operand_segment_sizes = dense<[1,1,1,0,0,0,0]> : vector<7xi32>, collapse_val = 2, ordered_val = 1} :
     (index, index, index) -> ()
 
   // CHECK: omp.wsloop (%{{.*}}) : index = (%{{.*}}) to (%{{.*}}) step (%{{.*}}) linear(%{{.*}} = %{{.*}} : memref<i32>) schedule(static)
   "omp.wsloop" (%lb, %ub, %step, %data_var, %linear_var) ({
     ^bb0(%iv: index):
       omp.yield
-  }) {operand_segment_sizes = dense<[1,1,1,1,1,0,0]> : vector<7xi32>, schedule_val = #omp<"schedulekind Static">} :
+  }) {operand_segment_sizes = dense<[1,1,1,1,1,0,0]> : vector<7xi32>, schedule_val = #omp<"schedulekind static">} :
     (index, index, index, memref<i32>, i32) -> ()
 
   // CHECK: omp.wsloop (%{{.*}}) : index = (%{{.*}}) to (%{{.*}}) step (%{{.*}}) linear(%{{.*}} = %{{.*}} : memref<i32>, %{{.*}} = %{{.*}} : memref<i32>) schedule(static)
   "omp.wsloop" (%lb, %ub, %step, %data_var, %data_var, %linear_var, %linear_var) ({
     ^bb0(%iv: index):
       omp.yield
-  }) {operand_segment_sizes = dense<[1,1,1,2,2,0,0]> : vector<7xi32>, schedule_val = #omp<"schedulekind Static">} :
+  }) {operand_segment_sizes = dense<[1,1,1,2,2,0,0]> : vector<7xi32>, schedule_val = #omp<"schedulekind static">} :
     (index, index, index, memref<i32>, memref<i32>, i32, i32) -> ()
 
   // CHECK: omp.wsloop (%{{.*}}) : index = (%{{.*}}) to (%{{.*}}) step (%{{.*}}) linear(%{{.*}} = %{{.*}} : memref<i32>) schedule(dynamic = %{{.*}}) collapse(3) ordered(2)
   "omp.wsloop" (%lb, %ub, %step, %data_var, %linear_var, %chunk_var) ({
     ^bb0(%iv: index):
       omp.yield
-  }) {operand_segment_sizes = dense<[1,1,1,1,1,0,1]> : vector<7xi32>, schedule_val = #omp<"schedulekind Dynamic">, collapse_val = 3, ordered_val = 2} :
+  }) {operand_segment_sizes = dense<[1,1,1,1,1,0,1]> : vector<7xi32>, schedule_val = #omp<"schedulekind dynamic">, collapse_val = 3, ordered_val = 2} :
     (index, index, index, memref<i32>, i32, i32) -> ()
 
   // CHECK: omp.wsloop (%{{.*}}) : index = (%{{.*}}) to (%{{.*}}) step (%{{.*}}) schedule(auto) nowait
   "omp.wsloop" (%lb, %ub, %step) ({
     ^bb0(%iv: index):
       omp.yield
-  }) {operand_segment_sizes = dense<[1,1,1,0,0,0,0]> : vector<7xi32>, nowait, schedule_val = #omp<"schedulekind Auto">} :
+  }) {operand_segment_sizes = dense<[1,1,1,0,0,0,0]> : vector<7xi32>, nowait, schedule_val = #omp<"schedulekind auto">} :
     (index, index, index) -> ()
 
   return
 }
 
 // CHECK-LABEL: omp_wsloop_pretty
 func @omp_wsloop_pretty(%lb : index, %ub : index, %step : index, %data_var : memref<i32>, %linear_var : i32, %chunk_var : i32, %chunk_var2 : i16) -> () {
 

mlir/test/mlir-tblgen/directive-common.td

def TDLC_ClauseA : Clause<"clausea"> {
   let enumClauseValue = "AKind";
   let allowedClauseValues = [
     TDLCV_vala,
     TDLCV_valb,
     TDLCV_valc
   ];
 }
 
-// CHECK: def AKindvala : I32EnumAttrCase<"vala", 0>;
-// CHECK: def AKindvalb : I32EnumAttrCase<"valb", 1>;
+// CHECK: def AKindvala : I32EnumAttrCase<"Vala", 0, "vala">;
+// CHECK: def AKindvalb : I32EnumAttrCase<"Valb", 1, "valb">;
 // CHECK: def AKind: I32EnumAttr<
 // CHECK:   "ClauseAKind",
 // CHECK:   "AKind Clause",
 // CHECK:   [AKindvala,AKindvalb]> {
 // CHECK:     let cppNamespace = "::mlir::tdl";
 // CHECK: }
 // CHECK: def AKindAttr : EnumAttr<TDL_Dialect, AKind, "akind">;

mlir/tools/mlir-tblgen/DirectiveCommonGen.cpp

for (const auto &r : clauses) {
     assert(!enumName.empty() && "enumClauseValue field not set.");
 
     std::vector<std::string> cvDefs;
     for (const auto &it : llvm::enumerate(clauseVals)) {
       ClauseVal cval{it.value()};
       if (!cval.isUserVisible())
         continue;
 
-      const auto name = cval.getFormattedName();
+      const std::string name = cval.getFormattedName();
+      std::string enumValName(name.length(), ' ');
+      std::transform(name.begin(), name.end(), enumValName.begin(),
+                     llvm::toLower);
+      enumValName[0] = llvm::toUpper(enumValName[0]);
       std::string cvDef{(enumName + llvm::Twine(name)).str()};
-      os << "def " << cvDef << " : I32EnumAttrCase<\"" << name << "\", "
-         << it.index() << ">;\n";
+      os << "def " << cvDef << " : I32EnumAttrCase<\"" << enumValName << "\", "
+         << it.index() << ", \"" << name << "\">;\n";
       cvDefs.push_back(cvDef);
     }
 
     os << "def " << enumName << ": I32EnumAttr<\n";
     os << "  \"Clause" << enumName << "\",\n";
     os << "  \"" << enumName << " Clause\",\n";
     os << "  [";
     for (unsigned int i = 0; i < cvDefs.size(); i++) {