[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Francesco Petrogalli via Phabricator via llvm-commits]

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