[PATCH] D72304: Summary: Add OpenMP Directives (master and critical) to OMPBuilder, and use them in clang.
Johannes Doerfert via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Jan 11 00:06:43 PST 2020
jdoerfert added a comment.
I added quite a bunch of inline comments again :( and some high level stuff below.
---
Commits need a message that explains the change.
Also the "Summary" should not be in the commit title.
---
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.
================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3005
+ llvm::BranchInst *IPBBTI =
+ llvm::dyn_cast<llvm::BranchInst>(IPBB->getTerminator());
+ llvm::BasicBlock *DestBB = IPBBTI->getSuccessor(0);
----------------
Can you add an assertion that the branch has only a single successor.
================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3009
+ // erase and replace with cleanup branch.
+ IPBB->getTerminator()->eraseFromParent();
+ Builder.SetInsertPoint(IPBB);
----------------
`IPBBTI->eraseFromParent()`
================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3020
+ if (AllocaIP.isSet())
+ AllocaInsertPt = &*AllocaIP.getPoint();
+ auto OldReturnBlock = ReturnBlock;
----------------
Why the `isSet` check? If we need it, then it's missing in other places as well (I think).
================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3060
+ ? nullptr
+ : Builder.CreateIntCast(EmitScalarExpr(Hint), CGM.Int32Ty, false);
+
----------------
Can we do
```
llvm::Value *HintInst = nullptr;
if (Hint)
...
```
these three lines look ugly :(
================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3076
+ EmitBranchThroughCleanup(Dest);
+ };
+
----------------
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.
================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3099
+ ReturnBlock = OldReturnBlock;
+ };
+
----------------
Same here, that looks like the other "BodyGenCB". We should really make them helpers.
================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:259
+ /// \param BodyGenCB Callback that will generate the region code.
+ ///
+ /// \returns The insertion position *after* the master.
----------------
FiniCB param docu is missing.
================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:295
+ BasicBlock *ExitBB,
+ bool conditional = false);
+
----------------
`Conditional`
================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:312
+ Instruction *ExitCall,
+ bool hasFinalize);
+
----------------
`HasFinalize`
================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:331
+
+ InsertPointTy EmitOMPInlinedRegion(omp::Directive OMPD,
+ Instruction *EntryCall,
----------------
Format: no newline, above the comments got broken
================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:349
+ const llvm::Twine &Name,
+ unsigned AddressSpace);
};
----------------
All methods need documentation of some kind. `llvm::` is not needed here. It seems one method is declared twice.
================
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
----------------
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.
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:674
+ SmallVector<llvm::Value *, 4> EnterArgs(std::begin(Args), std::end(Args));
+ Function *fn = nullptr;
+ if (HintInst) {
----------------
Spelling: `Fn`
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:695
+ BodyGenCallbackTy BodyGenCB, FinalizeCallbackTy FiniCB, bool conditional,
+ bool hasFinalize) {
+
----------------
Spelling `Conditional`, `HasFinalize`
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:700
+ // Create 'Critical' entry and body blocks, in preparation
+ // for conditional creation
+ BasicBlock *EntryBB = Builder.GetInsertBlock();
----------------
Outdated comment.
================
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 &&
----------------
Nit: `InsertPointTy FiniIP(FiniBB, FiniBB->getFirstInsertionPt());`
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:721
+ "Unexpected insertion point for finalization call!");
+ emitCommonDirectiveExit(OMPD, FinIP, ExitBB, ExitCall, /*hasFinalize*/ true);
+
----------------
Shouldn't we use the argument here?
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:728
+ SplitPos->eraseFromParent();
+ }
+
----------------
Style: No braces
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:737
+ omp::Directive omp, llvm::Value *EntryCall, BasicBlock *ExitBB,
+ bool conditional) {
+
----------------
`Conditional`, here and elsewhere no `llvm::` (and `omp::`)
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:739
+
+ OpenMPIRBuilder::InsertPointTy Contpt;
+ llvm::BasicBlock *EntryBB = Builder.GetInsertBlock();
----------------
Naming: `ContPt` or `ContIP` or ...
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:766
+ // otherwise Return an insertion point to current block
+ Contpt = Builder.saveIP();
+
----------------
I usually have neither or both conditional branches in braces. Here we can opt for an early exist (preferred anyway) and get:
```
if (!Conditional)
return Builder.SaveIP();
...
```
Some variables can then be moved past this early exit
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:790
+ Instruction *InsertBBTI = InsertBB->getTerminator();
+ if (!(InsertBBTI)) {
+ Builder.SetInsertPoint(InsertBB);
----------------
extra parenthesis, and, actually, this condition can not happen (I think). There is always something at the end of the block or your get an error earlier.
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:800
+ ExitCall->removeFromParent();
+ Builder.Insert(ExitCall);
+
----------------
`ExitCall->moveBefore(IP)`, potentially with some IP = ... above.
We should specify what the builder insert position is after this call, maybe "unspecified"?
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:808
+ StringRef FirstSeparator,
+ StringRef Separator) const {
+ SmallString<128> Buffer;
----------------
This can be a static helper and the name should be more descriptive, `getName` is very generic.
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:824
+ Out << Name;
+ StringRef RuntimeName = Out.str();
+ auto &Elem = *InternalVars.try_emplace(RuntimeName, nullptr).first;
----------------
This seems to be too much trouble to get a StringRef from a Twine. Maybe start with a StringRef and avoid all of it?
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:836
+ /*InsertBefore=*/nullptr, llvm::GlobalValue::NotThreadLocal,
+ AddressSpace);
+}
----------------
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;
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72304/new/
https://reviews.llvm.org/D72304
More information about the llvm-commits
mailing list