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

[Valentin Clement via Phabricator via llvm-commits]

clementval added inline comments.


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:212
+    OS << "\n";
+    std::string enumName{(llvm::Twine(R->getName().drop_front(Prefix.size())) +
+                          llvm::Twine("Kind"))
----------------
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. 


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


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:229
+      }
+      EnumHelperFuncs += enumName + " get" + enumName + "(StringRef);\n";
+    }
----------------
kiranchandramohan wrote:
> clementval wrote:
> > Do you want `enumName` twice in the name of your function?
> Yes, the first is the return type (enum) and the second is the name of the function. 
> 
> ProcBind getProcBind(StringRef)
Fine! I missed the space :-) 


================
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"))
----------------
kiranchandramohan wrote:
> clementval wrote:
> > Same comment here about the prefix. 
> Have provided an explanation above.
Same answer as above.


================
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:
> > 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. 


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

https://reviews.llvm.org/D84347