fpetrogalli added a comment.
Hi @jdoerfert , thank you for working on this!
I have added some minor comments.
Francesco
================
Comment at: clang/lib/CodeGen/CodeGenModule.h:593
+ llvm::OpenMPIRBuilder &getOpenMPIRBuilder() {
+ assert(OMPBuilder != nullptr);
+ return *OMPBuilder;
----------------
Nit: wouldn't the following be more informative? (and, also, I thought it was
preferred style for LLVM codebase)
```
assert(OMPBuilder && "Invalid OMPBuilder instance");
```
================
Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:27
+ /// IDs for all omp runtime library (RTL) functions.
+ enum RTLFnKind {
+#define OMP_RTL(Enum, ...) Enum,
----------------
I'd prefer use `enum class` instead of enums. If needed in some interface, it
make it easier to see the actual type instead of a plain `unsigned`. No strong
opinion though.
================
Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:24
+
+Function *OpenMPIRBuilder::getRuntimeFunction(RTLFnKind FnID) {
+ Function *Fn = nullptr;
----------------
Mark this method as `const`? It doesn't seem to change any of the fields of the
instance.
================
Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:29-48
+#define OMP_RTL(Enum, Str, IsVarArg, ReturnType, ...)
\
+ case Enum:
\
+ Fn = M.getFunction(Str);
\
+ break;
+#include "llvm/IR/OpenMPKinds.def"
+ }
+
----------------
Why not use `getorInsertFunction` directly instead of splitting the two cases?
================
Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:85
+
+Value *OpenMPIRBuilder::getIdent(Constant *SrcLocStr, unsigned LocFlags) {
+ // Enable "C-mode".
----------------
`const` method?
================
Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:113
+
+Constant *OpenMPIRBuilder::getSrcLocStr(std::string LocStr) {
+ Constant *&SrcLocStr = SrcLocStrMap[LocStr];
----------------
`const` method?
================
Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:131
+
+Constant *OpenMPIRBuilder::getDefaultSrcLocStr() {
+ return getSrcLocStr(";unknown;unknown;0;0;;");
----------------
`const` method?
================
Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:140
+
+Value *OpenMPIRBuilder::getThreadID(Value *Ident) {
+ // TODO: It makes only so much sense to actually cache the global_thread_num
----------------
`const` method?
================
Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:168
+
+void OpenMPIRBuilder::emitOMPBarrier(const LocationDescription &Loc,
+ DirektiveKind DK, bool CheckCancelFlag) {
----------------
`const` method?
================
Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:173
+ return emitBarrierImpl(Loc, DK, CheckCancelFlag,
+ /* ForceSimpleCall */ false);
+}
----------------
Nit!
```
/*ForceSimpleCall=*/ false);
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69785/new/
https://reviews.llvm.org/D69785
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
