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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

Some replies before addressing 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
----------------
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.


================
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:
> 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
  ];
}


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


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


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:229
+      }
+      EnumHelperFuncs += enumName + " get" + enumName + "(StringRef);\n";
+    }
----------------
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)


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


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

https://reviews.llvm.org/D84347