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

[Valentin Clement via Phabricator via llvm-commits]

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

LGTM. Thanks for making the changes.



================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:207
+  for (const auto &R : Records) {
+    const auto &ClauseVals = R->getValueAsListOfDefs("allowedClauseValues");
+    if (ClauseVals.size() <= 0)
----------------
kiranchandramohan wrote:
> 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.
Good move!


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:218
+      const auto &Value = CV->getValueAsInt("value");
+      OS << "  " << CV->getName() << "=" << Value << ",\n";
+    }
----------------
kiranchandramohan wrote:
> 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
> ];
Sounds good!


================
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.
----------------
kiranchandramohan wrote:
> 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.
Fine.


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

https://reviews.llvm.org/D84347