fghanim marked 10 inline comments as done.
fghanim added a comment.
> Commits need a message that explains the change.
> Also the "Summary" should not be in the commit title.
Is this something for future commits or do you want me to change these?
If the latter, how do I do that?
> This adds support for clang to build the directives via the OMPIRBuilder so
> we need to add tests for that. Probably "just" running existing codegen tests
> for the directives with the OMPIRBuidlder enabled.
You mean unit tests? or something else? If not unit tests, Where do I do that?
As for rest of comments, anything without response, is something I will do.
================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3020
+ if (AllocaIP.isSet())
+ AllocaInsertPt = &*AllocaIP.getPoint();
+ auto OldReturnBlock = ReturnBlock;
----------------
jdoerfert wrote:
> Why the `isSet` check? If we need it, then it's missing in other places as
> well (I think).
While I was writing the response I realized that I should've made it
` assert((!AllocaIP.isSet() || AllocaInsertPt == AllocaIP.getpoint()) &&
"Inlined directives allocas should be emitted to entry block of the inner most
containing outlined function"; `
or something less verbose.
Inlined directives should emit their `alloca`s to the entry block of the
containing outined function, right? For that reason, and since we don't store
an `alloca` insertion point in the OMPBuilder, I pass an empty insertion point,
and back up the current point, without changing it.
I was planning, in the future, to keep a copy of the most recent alloca
insertion point which we set when we outline, and pass that for `BodyCB`s
within inlined directives.
================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3060
+ ? nullptr
+ : Builder.CreateIntCast(EmitScalarExpr(Hint), CGM.Int32Ty, false);
+
----------------
jdoerfert wrote:
> Can we do
> ```
> llvm::Value *HintInst = nullptr;
> if (Hint)
> ...
> ```
> these three lines look ugly :(
np.
btw, the todo note is about the `CGM.Int32Ty`. Currently in clang it's
`CGM.IntTy`. I'll expand on the todo
================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3076
+ EmitBranchThroughCleanup(Dest);
+ };
+
----------------
jdoerfert wrote:
> I somehow have the feeling we could have a "FiniCB" helper function as they
> seem to be always the same. A TODO mentioning that is also OK for now.
I'll do a Todo for now. I am using a very similar `FiniCB` to the one you used
for `parallel`. So it doesn't make sense to change these here and not that one
with them.
================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3099
+ ReturnBlock = OldReturnBlock;
+ };
+
----------------
jdoerfert wrote:
> Same here, that looks like the other "BodyGenCB". We should really make them
> helpers.
Same as prev.
================
Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:68
+ // Type::getInt32Ty(M.getContext()),/*NumElements*/ 8);
+ // KmpCriticalNamePtrTy = PointerType::getUnqual(KmpCriticalNameTy);
// Create all simple and struct types exposed by the runtime and remember
----------------
jdoerfert wrote:
> These comments that reference `KmpCriticalNameTy` do not help here. If you
> want, feel free to add comments that explain how the result looks in a
> generic way but only showing `KmpCriticalNameTy` seems counterproductive to
> me.
I was testing something, and I forgot to remove them when I was done.
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:717
+ // emit exit call and do any needed finalization.
+ auto FinIP = InsertPointTy(FiniBB, FiniBB->getFirstInsertionPt());
+ assert(FiniBB->getTerminator()->getNumSuccessors() == 1 &&
----------------
jdoerfert wrote:
> Nit: `InsertPointTy FiniIP(FiniBB, FiniBB->getFirstInsertionPt());`
Nit?
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:721
+ "Unexpected insertion point for finalization call!");
+ emitCommonDirectiveExit(OMPD, FinIP, ExitBB, ExitCall, /*hasFinalize*/ true);
+
----------------
jdoerfert wrote:
> Shouldn't we use the argument here?
"The argument" refers to?
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:808
+ StringRef FirstSeparator,
+ StringRef Separator) const {
+ SmallString<128> Buffer;
----------------
jdoerfert wrote:
> This can be a static helper and the name should be more descriptive,
> `getName` is very generic.
This is taken from clang (I think CG_OMP) as is. I am keeping everything as is
for now in case somebody decided to help and came across it being used
somewhere else, they should recognize it immediately.
So for now, I suggest a todo. cos if I find that it's not used except with
`critical`, I'll merge all 3 functions into 1 and be done with it.
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:824
+ Out << Name;
+ StringRef RuntimeName = Out.str();
+ auto &Elem = *InternalVars.try_emplace(RuntimeName, nullptr).first;
----------------
jdoerfert wrote:
> This seems to be too much trouble to get a StringRef from a Twine. Maybe
> start with a StringRef and avoid all of it?
Same as prev.
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:836
+ /*InsertBefore=*/nullptr, llvm::GlobalValue::NotThreadLocal,
+ AddressSpace);
+}
----------------
jdoerfert wrote:
> Do we really want common linkage for "internal variables"? Internal or
> private would have been my first guess. Maybe the "internal" part is
> different than I expect it.
>
> ---
>
> FWIW, the usual pattern is
> ```
> if (!Elem.second)
> Elem.second = ...
> assert(...)
> return Elem.second;
> ```
same as prev.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72304/new/
https://reviews.llvm.org/D72304
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
