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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
