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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 16 inline comments as done.
jdoerfert added a comment.

In D69785#1732951 <https://reviews.llvm.org/D69785#1732951>, @rogfer01 wrote:

> I made a small experiment lowering a `taskwait` (which is even simpler than `barrier` in "lowering" complexity). It was pretty straightforward after all.


Nice, can you share the code? Was it based on the OpenMPIRBuilder (this one or the old one) or somehow separate?

> I have a few questions just to reassure I fully understand your vision here:

Sure thing.

> - the parameter `Loc` is currently unused but my understanding from the comment in `OpenMPIRBuilder::getSrcLocStr` eventually we will convert a `clang::SourceLocation` to a `llvm::Constant*` that represents the location as used by KMP, right?

It is not unused everywhere (emitOMPBarrier uses it) but you are right that some uses are missing. The idea is that it will combine information about the location in LLVM-IR (which it does already) and the location in the source. The latter is in Clang a `clang::SourceLocation` but I want a frontend agnostic interface that takes only the values we need. That is, we add something along the lines of `StringRef FileName; unsigned LineNo, ColumnNo;` to the struct and use these to create the string that goes into KMP.

> - emitting a `taskwait` however has this ```lang=cpp if (auto *Region = dyn_cast_or_null<CGOpenMPRegionInfo>(CGF.CapturedStmtInfo)) Region->emitUntiedSwitch(CGF); ``` but my understanding is that we can apply the same scheme to these hooks as well, recursively, i.e.: progressively convert them to calls to `OpenMPIRBuilder` (if needed under the flag suggested above by Alexey). Does this make sense?

Yes. Eventually, the logic to create "untied switches" should move, then the logic surrounding it, ...
Though, I doubt we should start there as it will cause too much interaction with the internals of Clang.
Instead, we port task created (tied ones only), then add code that keeps track of untied tasks, then add the switches, ...

Does that makes sense?



================
Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:51
+  /// potentially other helpers into the underlying module. Must be called
+  /// before any other other method and only once!
+  void initialize();
----------------
fghanim wrote:
> ///before any other method and only once!
Agreed.


================
Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:72
+                      bool CheckCancelFlag = true);
+
+  ///}
----------------
fghanim wrote:
> Suggestion: Rename to createOMPBarrier( ... ); - similar naming scheme to IRBuilder, MDBuilder, etc.
Fine with me.


================
Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:24
+
+Function *OpenMPIRBuilder::getRuntimeFunction(RTLFnKind FnID) {
+  Function *Fn = nullptr;
----------------
fpetrogalli wrote:
> Mark this method as `const`? It doesn't seem to change any of the fields of the instance.
Some methods could be made const (and I will) but most of them not (they modify maps and the builder).
Making them const has only little benefit but I'll add it where appropriate anyway. There is little benefit because we basically do not hand out a `const OpenMPIRBuilder` to anyone anyway.


================
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"
+  }
+
----------------
fpetrogalli wrote:
> Why not use `getorInsertFunction` directly instead of splitting the two cases?
because this way we can define attributes easily for a subset of RT declarations separate from the type definition. Arguably, we can merge everything into a big macro that defines name, type and attributes but I figured this is easier to maintain. (The cost is not important as we pay it once per function at most and the switches should be eliminated). Can be changed later if we determine attributes will become part of all runtime function declarations.


================
Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:177
+void OpenMPIRBuilder::emitBarrierImpl(const LocationDescription &Loc,
+                                      DirektiveKind Kind, bool CheckCancelFlag,
+                                      bool ForceSimpleCall) {
----------------
fghanim wrote:
> DirectiveKind
(No need to repeat such comments, I will change the type name everywhere once I update ;))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785