[PATCH] D84347: [MLIR,OpenMP] Lowering of parallel operation: proc_bind clause 2/n

[Valentin Clement via Phabricator via llvm-commits]

clementval added a comment.

Added few comments but generally it looks good. Can you maybe complete the test `directive1.td` or `directive2.td` with a `ClauseVal` so we can check the generated code in the test suite.



================
Comment at: llvm/include/llvm/Frontend/Directive/DirectiveBase.td:87
 
-  // Set directive used by default when unknown. Function returning the kind
+  // Set clause used by default when unknown. Function returning the kind
   // of enumeration will use this clause as the default.
----------------
Thanks for the fix. Can be push without review. 


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:212
+    OS << "\n";
+    std::string enumName{(llvm::Twine(R->getName().drop_front(Prefix.size())) +
+                          llvm::Twine("Kind"))
----------------
What happens if we omit the prefix `OMPC_` when we define the ClauseVal ... name will be truncated right? I guess you could define the ClauseVal without the prefix in the `.td` file and drop the string manipulation here. It will probably be safer.  


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:218
+      const auto &Value = CV->getValueAsInt("value");
+      OS << "  " << CV->getName() << "=" << Value << ",\n";
+    }
----------------
Do you assume names have no spaces? Otherwise you should do the same trick than for the directive and clauses to add a `_` for the enum name. 


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:229
+      }
+      EnumHelperFuncs += enumName + " get" + enumName + "(StringRef);\n";
+    }
----------------
Do you want `enumName` twice in the name of your function?


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:349
   if (DefaultIt == Records.end()) {
-    PrintError("A least one " + Enum + " must be defined as default.");
+    PrintError("At least one " + Enum + " must be defined as default.");
     return;
----------------
Can be push without review. 


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:396
+    const auto DefaultName = (*DefaultIt)->getName();
+    std::string EnumName{(llvm::Twine(R->getName().drop_front(Prefix.size())) +
+                          llvm::Twine("Kind"))
----------------
Same comment here about the prefix. 


================
Comment at: mlir/tools/mlir-tblgen/OpenMPCommonGen.cpp:9
+//
+// OpenMPCommonGen generates utility information from the single OpenMP source
+// of truth in llvm/lib/Frontend/OpenMP.
----------------
Is this new tablegen specific to OpenMP or do you think we can use it for other directive language (e.g. OpenACC). No need to change this now, we can do when we see a need. Just asking your opinion. 


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

https://reviews.llvm.org/D84347