Page MenuHomePhabricator
Differential D82470 Diff 281939 llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

Changeset View

Standalone View

View Options

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

Show First 20 Lines • Show All 388 Lines • ▼ Show 20 Linesvoid OpenMPIRBuilder::emitCancelationCheckImpl(
auto &FI = FinalizationStack.back(); auto &FI = FinalizationStack.back();
FI.FiniCB(Builder.saveIP()); FI.FiniCB(Builder.saveIP());
// The continuation block is where code generation continues. // The continuation block is where code generation continues.
Builder.SetInsertPoint(NonCancellationBlock, NonCancellationBlock->begin()); Builder.SetInsertPoint(NonCancellationBlock, NonCancellationBlock->begin());
} }
IRBuilder<>::InsertPoint OpenMPIRBuilder::CreateParallel( IRBuilder<>::InsertPoint OpenMPIRBuilder::CreateParallel(
const LocationDescription &Loc, BodyGenCallbackTy BodyGenCB, const LocationDescription &Loc, InsertPointTy OuterAllocaIP,
PrivatizeCallbackTy PrivCB, FinalizeCallbackTy FiniCB, Value *IfCondition, BodyGenCallbackTy BodyGenCB, PrivatizeCallbackTy PrivCB,
Value *NumThreads, omp::ProcBindKind ProcBind, bool IsCancellable) { FinalizeCallbackTy FiniCB, Value *IfCondition, Value *NumThreads,
omp::ProcBindKind ProcBind, bool IsCancellable) {
if (!updateToLocation(Loc)) if (!updateToLocation(Loc))
return Loc.IP; return Loc.IP;
Constant *SrcLocStr = getOrCreateSrcLocStr(Loc); Constant *SrcLocStr = getOrCreateSrcLocStr(Loc);
Value *Ident = getOrCreateIdent(SrcLocStr); Value *Ident = getOrCreateIdent(SrcLocStr);
Value *ThreadID = getOrCreateThreadID(Ident); Value *ThreadID = getOrCreateThreadID(Ident);
if (NumThreads) { if (NumThreads) {
Show All 16 LinesIRBuilder<>::InsertPoint OpenMPIRBuilder::CreateParallel(
BasicBlock *InsertBB = Builder.GetInsertBlock(); BasicBlock *InsertBB = Builder.GetInsertBlock();
Function *OuterFn = InsertBB->getParent(); Function *OuterFn = InsertBB->getParent();
// Vector to remember instructions we used only during the modeling but which // Vector to remember instructions we used only during the modeling but which
// we want to delete at the end. // we want to delete at the end.
SmallVector<Instruction *, 4> ToBeDeleted; SmallVector<Instruction *, 4> ToBeDeleted;
Builder.SetInsertPoint(OuterFn->getEntryBlock().getFirstNonPHI()); // Change the location to the outer alloca insertion point to create and
// initialize the allocas we pass into the parallel region.
Builder.restoreIP(OuterAllocaIP);
AllocaInst *TIDAddr = Builder.CreateAlloca(Int32, nullptr, "tid.addr"); AllocaInst *TIDAddr = Builder.CreateAlloca(Int32, nullptr, "tid.addr");
AllocaInst *ZeroAddr = Builder.CreateAlloca(Int32, nullptr, "zero.addr"); AllocaInst *ZeroAddr = Builder.CreateAlloca(Int32, nullptr, "zero.addr");
fghanimUnsubmitted
Not Done
ReplyInline Actions

Here and elsewhere: You seem to have forgotten to undo the changes introduced by using a separate AllocaBuilder?

Also, Nit: before using AllocaIP, either add an assert that it is not empty, or alternatively, add logic to just set it to entry block of outerfn if it is

fghanim: Here and elsewhere: You seem to have forgotten to undo the changes introduced by using a…
jdoerfertAuthorUnsubmitted
Done
ReplyInline Actions

Here and elsewhere: You seem to have forgotten to undo the changes introduced by using a separate AllocaBuilder?

I don't get this. These changes are on purpose.

jdoerfert: > Here and elsewhere: You seem to have forgotten to undo the changes introduced by using a…
fghanimUnsubmitted
Not Done
ReplyInline Actions

Oh, my bad. I assumed that since we now pass allocaip to communicate where is the insertion point, using builder the way we used to is sufficient, and this was leftover code. So now what purpose does AllocaBuilder serve?

fghanim: Oh, my bad. I assumed that since we now pass `allocaip` to communicate where is the insertion…
jdoerfertAuthorUnsubmitted
Done
ReplyInline Actions
  1. No switching the IP back and forth in one builder all the time.
  2. Showing by the name of the builder directly where the instructions will be created.
jdoerfert: 1) No switching the IP back and forth in one builder all the time. 2) Showing by the name of…
fghanimUnsubmitted
Not Done
ReplyInline Actions

No switching the IP back and forth in one builder all the time.

  • Is this about something you expect in the future? because unless I am missing something, and looking at it side by side, I don't see any less setting /reseting of IPs going on, except in the new version we to set the debug info IP in allocabuilder in addition to in builder.
  • we don't use either AllocaIP after returning from bodyCB
  • Even if we did, both bodyCB, and FiniCB have IPGuards inside them.
  • Also, since IRBuilders do insertbefore an IP, both AllocaIP should be up-to-date except if the iterator for an IP gets invalidated, the risk of which is sort of proportional to increase in number of IRbuilders.

Showing by the name of the builder directly where the instructions will be created.

Wouldn't a comment be better?
Personally, I find it confusing when something called AllocaBuilder, is being used to emit loads and stores.

fghanim: >No switching the IP back and forth in one builder all the time. - Is this about something you…
// If there is an if condition we actually use the TIDAddr and ZeroAddr in the // If there is an if condition we actually use the TIDAddr and ZeroAddr in the
// program, otherwise we only need them for modeling purposes to get the // program, otherwise we only need them for modeling purposes to get the
// associated arguments in the outlined function. In the former case, // associated arguments in the outlined function. In the former case,
// initialize the allocas properly, in the latter case, delete them later. // initialize the allocas properly, in the latter case, delete them later.
if (IfCondition) { if (IfCondition) {
Builder.CreateStore(Constant::getNullValue(Int32), TIDAddr); Builder.CreateStore(Constant::getNullValue(Int32), TIDAddr);
Builder.CreateStore(Constant::getNullValue(Int32), ZeroAddr); Builder.CreateStore(Constant::getNullValue(Int32), ZeroAddr);
Show All 33 Lines assert(IP.getBlock()->getTerminator()->getNumSuccessors() == 1 &&
"Unexpected insertion point for finalization call!"); "Unexpected insertion point for finalization call!");
return FiniCB(IP); return FiniCB(IP);
}; };
FinalizationStack.push_back({FiniCBWrapper, OMPD_parallel, IsCancellable}); FinalizationStack.push_back({FiniCBWrapper, OMPD_parallel, IsCancellable});
// Generate the privatization allocas in the block that will become the entry // Generate the privatization allocas in the block that will become the entry
// of the outlined function. // of the outlined function.
InsertPointTy AllocaIP(PRegEntryBB, Builder.SetInsertPoint(PRegEntryBB->getTerminator());
PRegEntryBB->getTerminator()->getIterator()); InsertPointTy InnerAllocaIP = Builder.saveIP();
Builder.restoreIP(AllocaIP);
fghanimUnsubmitted
Not Done
ReplyInline Actions

NIT: I think it would be better to give each a different name, to avoid unnecessary confusion. maybe OuterAllocaIP and innerAllocaIP? Also, we don't overwrite the outerAllocaIP in case it's needed later?

fghanim: NIT: I think it would be better to give each a different name, to avoid unnecessary confusion.
AllocaInst *PrivTIDAddr = AllocaInst *PrivTIDAddr =
Builder.CreateAlloca(Int32, nullptr, "tid.addr.local"); Builder.CreateAlloca(Int32, nullptr, "tid.addr.local");
Instruction *PrivTID = Builder.CreateLoad(PrivTIDAddr, "tid"); Instruction *PrivTID = Builder.CreateLoad(PrivTIDAddr, "tid");
// Add some fake uses for OpenMP provided arguments. // Add some fake uses for OpenMP provided arguments.
ToBeDeleted.push_back(Builder.CreateLoad(TIDAddr, "tid.addr.use")); ToBeDeleted.push_back(Builder.CreateLoad(TIDAddr, "tid.addr.use"));
ToBeDeleted.push_back(Builder.CreateLoad(ZeroAddr, "zero.addr.use")); ToBeDeleted.push_back(Builder.CreateLoad(ZeroAddr, "zero.addr.use"));
Show All 12 LinesIRBuilder<>::InsertPoint OpenMPIRBuilder::CreateParallel(
// PRegionExitBB <- A common exit to simplify block collection. // PRegionExitBB <- A common exit to simplify block collection.
// //
LLVM_DEBUG(dbgs() << "Before body codegen: " << *OuterFn << "\n"); LLVM_DEBUG(dbgs() << "Before body codegen: " << *OuterFn << "\n");
// Let the caller create the body. // Let the caller create the body.
assert(BodyGenCB && "Expected body generation callback!"); assert(BodyGenCB && "Expected body generation callback!");
InsertPointTy CodeGenIP(PRegBodyBB, PRegBodyBB->begin()); InsertPointTy CodeGenIP(PRegBodyBB, PRegBodyBB->begin());
BodyGenCB(AllocaIP, CodeGenIP, *PRegPreFiniBB); BodyGenCB(InnerAllocaIP, CodeGenIP, *PRegPreFiniBB);
LLVM_DEBUG(dbgs() << "After body codegen: " << *OuterFn << "\n"); LLVM_DEBUG(dbgs() << "After body codegen: " << *OuterFn << "\n");
FunctionCallee RTLFn = getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_fork_call); FunctionCallee RTLFn = getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_fork_call);
if (auto *F = dyn_cast<llvm::Function>(RTLFn.getCallee())) { if (auto *F = dyn_cast<llvm::Function>(RTLFn.getCallee())) {
if (!F->hasMetadata(llvm::LLVMContext::MD_callback)) { if (!F->hasMetadata(llvm::LLVMContext::MD_callback)) {
llvm::LLVMContext &Ctx = F->getContext(); llvm::LLVMContext &Ctx = F->getContext();
MDBuilder MDB(Ctx); MDBuilder MDB(Ctx);
▲ Show 20 Lines • Show All 142 Lines • ▼ Show 20 Lines for (Use &U : V.uses())
Uses.push_back(&U); Uses.push_back(&U);
Value *ReplacementValue = nullptr; Value *ReplacementValue = nullptr;
CallInst *CI = dyn_cast<CallInst>(&V); CallInst *CI = dyn_cast<CallInst>(&V);
if (CI && CI->getCalledFunction() == TIDRTLFn.getCallee()) { if (CI && CI->getCalledFunction() == TIDRTLFn.getCallee()) {
ReplacementValue = PrivTID; ReplacementValue = PrivTID;
} else { } else {
Builder.restoreIP( Builder.restoreIP(
PrivCB(AllocaIP, Builder.saveIP(), V, ReplacementValue)); PrivCB(InnerAllocaIP, Builder.saveIP(), V, ReplacementValue));
assert(ReplacementValue && assert(ReplacementValue &&
"Expected copy/create callback to set replacement value!"); "Expected copy/create callback to set replacement value!");
if (ReplacementValue == &V) if (ReplacementValue == &V)
return; return;
} }
for (Use *UPtr : Uses) for (Use *UPtr : Uses)
UPtr->set(ReplacementValue); UPtr->set(ReplacementValue);
}; };
for (Value *Input : Inputs) { for (Value *Input : Inputs) {
LLVM_DEBUG(dbgs() << "Captured input: " << *Input << "\n"); LLVM_DEBUG(dbgs() << "Captured input: " << *Input << "\n");
PrivHelper(*Input); PrivHelper(*Input);
} }
LLVM_DEBUG({
for (Value *Output : Outputs)
LLVM_DEBUG(dbgs() << "Captured output: " << *Output << "\n");
});
assert(Outputs.empty() && assert(Outputs.empty() &&
"OpenMP outlining should not produce live-out values!"); "OpenMP outlining should not produce live-out values!");
LLVM_DEBUG(dbgs() << "After privatization: " << *OuterFn << "\n"); LLVM_DEBUG(dbgs() << "After privatization: " << *OuterFn << "\n");
LLVM_DEBUG({ LLVM_DEBUG({
for (auto *BB : Blocks) for (auto *BB : Blocks)
dbgs() << " PBR: " << BB->getName() << "\n"; dbgs() << " PBR: " << BB->getName() << "\n";
}); });
▲ Show 20 Lines • Show All 432 Lines • Show Last 20 Lines