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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:80
-#include "llvm/Frontend/OpenMP/OMPKinds.def"
-
 /// IDs for all omp runtime library ident_t flag encodings (see
----------------
kiranchandramohan wrote:
> jdoerfert wrote:
> > We can delete `OMP_PROC_BIND_KIND` from OpenMPKinds.def now, right?
> Good Catch.
> 
> There is a usage in clang/lib/Basic/OpenMPKinds.cpp. I will try to remove that also.
Removing the usage in Clang would require making changes for all Clauses having enums. Can do that as a separate patch. Is that OK?


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:92
 
+ProcBindKind getProcBindKind(llvm::StringRef Str);
+
----------------
jdoerfert wrote:
> Documentation please.
Is this a doxygen documentation you are asking for?


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:207
+  for (const auto &R : Records) {
+    const auto &ClauseVals = R->getValueAsListOfDefs("allowedClauseValues");
+    if (ClauseVals.size() <= 0)
----------------
clementval wrote:
> We now have small wrapper to access record information -> https://github.com/llvm/llvm-project/blob/master/llvm/utils/TableGen/DirectiveEmitter.cpp#L135
> 
> Can you use them and add one for ClauseVal?
Done and moved all wrappers to llvm/include/llvm/TableGen/DirectiveEmitter.h so that they can be used in MLIR also.


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:212
+    OS << "\n";
+    std::string enumName{(llvm::Twine(R->getName().drop_front(Prefix.size())) +
+                          llvm::Twine("Kind"))
----------------
clementval wrote:
> kiranchandramohan wrote:
> > kiranchandramohan wrote:
> > > clementval wrote:
> > > > clementval wrote:
> > > > > 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.  
> > > > The clang-tidy rule for TableGen code force a capital latter for variable. So `enumName` should be `EnumName`. Can you update this and other places if needed?
> > > The OMPC_ prefix is there for the clauses. Have not used it for the ClauseVals. In this case, we have the following definition for the ProcBind clause. The new code is generating the name of the enumeration "ProcBindKind". Because the prefix is also a setting in the tablegen file I felt it is safe to use. Is that OK?
> > > 
> > > def OMPC_ProcBind : Clause<"proc_bind"> {
> > >   let clangClass = "OMPProcBindClause";
> > >   let allowedClauseValues = [
> > >     OMP_PROC_BIND_master,
> > >     OMP_PROC_BIND_close,
> > >     OMP_PROC_BIND_spread,
> > >     OMP_PROC_BIND_default,
> > >     OMP_PROC_BIND_unknown
> > >   ];
> > > }
> > will change.
> The prefix in the DirectiveLanguage record doesn't apply for the clause or directive records found in the tablegen file. It is just used for the code generation. It is a coincidence that the records use the same prefix in their name. At least their is no enforcement that the records follow the given prefix for directive or clause. So it might be risky. 
Now using a field in the Clause Record to get the name of enum class.
let enumClauseValue = "ProcBindKind";


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:218
+      const auto &Value = CV->getValueAsInt("value");
+      OS << "  " << CV->getName() << "=" << Value << ",\n";
+    }
----------------
clementval wrote:
> kiranchandramohan wrote:
> > clementval wrote:
> > > 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. 
> > Yes, that is the assumption.
> > BTW, Is that what the formattedName function does? 
> > 
> > Will change.
> Yes the function does that. 
I am using getFormattedName in some places now. But not for this specific instance. Here i am using the name of the record as the enumeration value. I did not want to create another field just for the name of the enumeration value. Specifically, i am using OMP_PROC_BIND_master etc as the enum value name. These will not have spaces in them.

let clangClass = "OMPProcBindClause";
let allowedClauseValues = [
  OMP_PROC_BIND_master,
  OMP_PROC_BIND_close,
  OMP_PROC_BIND_spread,
  OMP_PROC_BIND_default,
  OMP_PROC_BIND_unknown
];


================
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.
----------------
clementval wrote:
> kiranchandramohan wrote:
> > clementval wrote:
> > > 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. 
> > No, nothing specific to OpenMP now. Can rename to Directive.
> Up to you. As I said, this can be done when and if we see a need to share this with OpenACC for example. 
OK. not making changes now. We can rename when one more user comes up.


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

https://reviews.llvm.org/D84347