Skip to content

Commit b93f7db

Browse files
ergawyergawy
committed
Handle review comments
1 parent 3c1af06 commit b93f7db

File tree

1 file changed

+52
-56
lines changed

1 file changed

+52
-56
lines changed

‎mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

Lines changed: 52 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -701,34 +701,33 @@ convertOmpCritical(Operation &opInst, llvm::IRBuilderBase &builder,
701701
struct PrivateVarsInfo {
702702
template <typename OP>
703703
PrivateVarsInfo(OP op)
704-
: privateBlockArgs(
704+
: blockArgs(
705705
cast<omp::BlockArgOpenMPOpInterface>(*op).getPrivateBlockArgs()) {
706-
mlirPrivateVars.reserve(privateBlockArgs.size());
707-
llvmPrivateVars.reserve(privateBlockArgs.size());
708-
collectPrivatizationDecls<OP>(op, privateDecls);
706+
mlirVars.reserve(blockArgs.size());
707+
llvmVars.reserve(blockArgs.size());
708+
collectPrivatizationDecls<OP>(op);
709709

710710
for (mlir::Value privateVar : op.getPrivateVars())
711-
mlirPrivateVars.push_back(privateVar);
711+
mlirVars.push_back(privateVar);
712712
}
713713

714-
MutableArrayRef<BlockArgument> privateBlockArgs;
715-
SmallVector<mlir::Value> mlirPrivateVars;
716-
SmallVector<llvm::Value *> llvmPrivateVars;
717-
SmallVector<omp::PrivateClauseOp> privateDecls;
714+
MutableArrayRef<BlockArgument> blockArgs;
715+
SmallVector<mlir::Value> mlirVars;
716+
SmallVector<llvm::Value *> llvmVars;
717+
SmallVector<omp::PrivateClauseOp> privatizers;
718718

719719
private:
720720
/// Populates `privatizations` with privatization declarations used for the
721721
/// given op.
722722
template <class OP>
723-
static void collectPrivatizationDecls(
724-
OP op, SmallVectorImpl<omp::PrivateClauseOp> &privatizations) {
723+
void collectPrivatizationDecls(OP op) {
725724
std::optional<ArrayAttr> attr = op.getPrivateSyms();
726725
if (!attr)
727726
return;
728727

729-
privatizations.reserve(privatizations.size() + attr->size());
728+
privatizers.reserve(privatizers.size() + attr->size());
730729
for (auto symbolRef : attr->getAsRange<SymbolRefAttr>()) {
731-
privatizations.push_back(findPrivatizer(op, symbolRef));
730+
privatizers.push_back(findPrivatizer(op, symbolRef));
732731
}
733732
}
734733
};
@@ -1408,16 +1407,15 @@ initPrivateVars(llvm::IRBuilderBase &builder,
14081407
LLVM::ModuleTranslation &moduleTranslation,
14091408
PrivateVarsInfo &privateVarsInfo,
14101409
llvm::DenseMap<Value, Value> *mappedPrivateVars = nullptr) {
1411-
if (privateVarsInfo.privateBlockArgs.empty())
1410+
if (privateVarsInfo.blockArgs.empty())
14121411
return llvm::Error::success();
14131412

14141413
llvm::BasicBlock *privInitBlock = splitBB(builder, true, "omp.private.init");
14151414
setInsertPointForPossiblyEmptyBlock(builder, privInitBlock);
14161415

14171416
for (auto [idx, zip] : llvm::enumerate(llvm::zip_equal(
1418-
privateVarsInfo.privateDecls, privateVarsInfo.mlirPrivateVars,
1419-
privateVarsInfo.privateBlockArgs,
1420-
privateVarsInfo.llvmPrivateVars))) {
1417+
privateVarsInfo.privatizers, privateVarsInfo.mlirVars,
1418+
privateVarsInfo.blockArgs, privateVarsInfo.llvmVars))) {
14211419
auto [privDecl, mlirPrivVar, blockArg, llvmPrivateVar] = zip;
14221420
llvm::Expected<llvm::Value *> privVarOrErr = initPrivateVar(
14231421
builder, moduleTranslation, privDecl, mlirPrivVar, blockArg,
@@ -1467,9 +1465,9 @@ allocatePrivateVars(llvm::IRBuilderBase &builder,
14671465
->getDataLayout()
14681466
.getProgramAddressSpace();
14691467

1470-
for (auto [privDecl, mlirPrivVar, blockArg] : llvm::zip_equal(
1471-
privateVarsInfo.privateDecls, privateVarsInfo.mlirPrivateVars,
1472-
privateVarsInfo.privateBlockArgs)) {
1468+
for (auto [privDecl, mlirPrivVar, blockArg] :
1469+
llvm::zip_equal(privateVarsInfo.privatizers, privateVarsInfo.mlirVars,
1470+
privateVarsInfo.blockArgs)) {
14731471
llvm::Type *llvmAllocType =
14741472
moduleTranslation.convertType(privDecl.getType());
14751473
builder.SetInsertPoint(allocaIP.getBlock()->getTerminator());
@@ -1479,7 +1477,7 @@ allocatePrivateVars(llvm::IRBuilderBase &builder,
14791477
llvmPrivateVar = builder.CreateAddrSpaceCast(llvmPrivateVar,
14801478
builder.getPtrTy(defaultAS));
14811479

1482-
privateVarsInfo.llvmPrivateVars.push_back(llvmPrivateVar);
1480+
privateVarsInfo.llvmVars.push_back(llvmPrivateVar);
14831481
}
14841482

14851483
return afterAllocas;
@@ -1909,7 +1907,7 @@ convertOmpTaskOp(omp::TaskOp taskOp, llvm::IRBuilderBase &builder,
19091907

19101908
PrivateVarsInfo privateVarsInfo(taskOp);
19111909
TaskContextStructManager taskStructMgr{builder, moduleTranslation,
1912-
privateVarsInfo.privateDecls};
1910+
privateVarsInfo.privatizers};
19131911

19141912
// Allocate and copy private variables before creating the task. This avoids
19151913
// accessing invalid memory if (after this scope ends) the private variables
@@ -1968,9 +1966,8 @@ convertOmpTaskOp(omp::TaskOp taskOp, llvm::IRBuilderBase &builder,
19681966
taskStructMgr.createGEPsToPrivateVars();
19691967

19701968
for (auto [privDecl, mlirPrivVar, blockArg, llvmPrivateVarAlloc] :
1971-
llvm::zip_equal(privateVarsInfo.privateDecls,
1972-
privateVarsInfo.mlirPrivateVars,
1973-
privateVarsInfo.privateBlockArgs,
1969+
llvm::zip_equal(privateVarsInfo.privatizers, privateVarsInfo.mlirVars,
1970+
privateVarsInfo.blockArgs,
19741971
taskStructMgr.getLLVMPrivateVarGEPs())) {
19751972
// To be handled inside the task.
19761973
if (!privDecl.readsFromMold())
@@ -2010,8 +2007,8 @@ convertOmpTaskOp(omp::TaskOp taskOp, llvm::IRBuilderBase &builder,
20102007
// firstprivate copy region
20112008
setInsertPointForPossiblyEmptyBlock(builder, copyBlock);
20122009
if (failed(copyFirstPrivateVars(
2013-
builder, moduleTranslation, privateVarsInfo.mlirPrivateVars,
2014-
taskStructMgr.getLLVMPrivateVarGEPs(), privateVarsInfo.privateDecls)))
2010+
builder, moduleTranslation, privateVarsInfo.mlirVars,
2011+
taskStructMgr.getLLVMPrivateVarGEPs(), privateVarsInfo.privatizers)))
20152012
return llvm::failure();
20162013

20172014
// Set up for call to createTask()
@@ -2028,11 +2025,10 @@ convertOmpTaskOp(omp::TaskOp taskOp, llvm::IRBuilderBase &builder,
20282025
builder.restoreIP(codegenIP);
20292026

20302027
llvm::BasicBlock *privInitBlock = nullptr;
2031-
privateVarsInfo.llvmPrivateVars.resize(
2032-
privateVarsInfo.privateBlockArgs.size());
2028+
privateVarsInfo.llvmVars.resize(privateVarsInfo.blockArgs.size());
20332029
for (auto [i, zip] : llvm::enumerate(llvm::zip_equal(
2034-
privateVarsInfo.privateBlockArgs, privateVarsInfo.privateDecls,
2035-
privateVarsInfo.mlirPrivateVars))) {
2030+
privateVarsInfo.blockArgs, privateVarsInfo.privatizers,
2031+
privateVarsInfo.mlirVars))) {
20362032
auto [blockArg, privDecl, mlirPrivVar] = zip;
20372033
// This is handled before the task executes
20382034
if (privDecl.readsFromMold())
@@ -2051,25 +2047,25 @@ convertOmpTaskOp(omp::TaskOp taskOp, llvm::IRBuilderBase &builder,
20512047
if (!privateVarOrError)
20522048
return privateVarOrError.takeError();
20532049
moduleTranslation.mapValue(blockArg, privateVarOrError.get());
2054-
privateVarsInfo.llvmPrivateVars[i] = privateVarOrError.get();
2050+
privateVarsInfo.llvmVars[i] = privateVarOrError.get();
20552051
}
20562052

20572053
taskStructMgr.createGEPsToPrivateVars();
20582054
for (auto [i, llvmPrivVar] :
20592055
llvm::enumerate(taskStructMgr.getLLVMPrivateVarGEPs())) {
20602056
if (!llvmPrivVar) {
2061-
assert(privateVarsInfo.llvmPrivateVars[i] &&
2057+
assert(privateVarsInfo.llvmVars[i] &&
20622058
"This is added in the loop above");
20632059
continue;
20642060
}
2065-
privateVarsInfo.llvmPrivateVars[i] = llvmPrivVar;
2061+
privateVarsInfo.llvmVars[i] = llvmPrivVar;
20662062
}
20672063

20682064
// Find and map the addresses of each variable within the task context
20692065
// structure
2070-
for (auto [blockArg, llvmPrivateVar, privateDecl] : llvm::zip_equal(
2071-
privateVarsInfo.privateBlockArgs, privateVarsInfo.llvmPrivateVars,
2072-
privateVarsInfo.privateDecls)) {
2066+
for (auto [blockArg, llvmPrivateVar, privateDecl] :
2067+
llvm::zip_equal(privateVarsInfo.blockArgs, privateVarsInfo.llvmVars,
2068+
privateVarsInfo.privatizers)) {
20732069
// This was handled above.
20742070
if (!privateDecl.readsFromMold())
20752071
continue;
@@ -2091,8 +2087,8 @@ convertOmpTaskOp(omp::TaskOp taskOp, llvm::IRBuilderBase &builder,
20912087
builder.SetInsertPoint(continuationBlockOrError.get()->getTerminator());
20922088

20932089
if (failed(cleanupPrivateVars(builder, moduleTranslation, taskOp.getLoc(),
2094-
privateVarsInfo.llvmPrivateVars,
2095-
privateVarsInfo.privateDecls)))
2090+
privateVarsInfo.llvmVars,
2091+
privateVarsInfo.privatizers)))
20962092
return llvm::make_error<PreviouslyReportedError>();
20972093

20982094
// Free heap allocated task context structure at the end of the task.
@@ -2221,8 +2217,8 @@ convertOmpWsloop(Operation &opInst, llvm::IRBuilderBase &builder,
22212217
return failure();
22222218

22232219
if (failed(copyFirstPrivateVars(
2224-
builder, moduleTranslation, privateVarsInfo.mlirPrivateVars,
2225-
privateVarsInfo.llvmPrivateVars, privateVarsInfo.privateDecls)))
2220+
builder, moduleTranslation, privateVarsInfo.mlirVars,
2221+
privateVarsInfo.llvmVars, privateVarsInfo.privatizers)))
22262222
return failure();
22272223

22282224
assert(afterAllocas.get()->getSinglePredecessor());
@@ -2275,8 +2271,8 @@ convertOmpWsloop(Operation &opInst, llvm::IRBuilderBase &builder,
22752271
return failure();
22762272

22772273
return cleanupPrivateVars(builder, moduleTranslation, wsloopOp.getLoc(),
2278-
privateVarsInfo.llvmPrivateVars,
2279-
privateVarsInfo.privateDecls);
2274+
privateVarsInfo.llvmVars,
2275+
privateVarsInfo.privatizers);
22802276
}
22812277

22822278
/// Converts the OpenMP parallel operation to LLVM IR.
@@ -2333,8 +2329,8 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
23332329
return llvm::make_error<PreviouslyReportedError>();
23342330

23352331
if (failed(copyFirstPrivateVars(
2336-
builder, moduleTranslation, privateVarsInfo.mlirPrivateVars,
2337-
privateVarsInfo.llvmPrivateVars, privateVarsInfo.privateDecls)))
2332+
builder, moduleTranslation, privateVarsInfo.mlirVars,
2333+
privateVarsInfo.llvmVars, privateVarsInfo.privatizers)))
23382334
return llvm::make_error<PreviouslyReportedError>();
23392335

23402336
if (failed(
@@ -2416,8 +2412,8 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
24162412
"failed to inline `cleanup` region of `omp.declare_reduction`");
24172413

24182414
if (failed(cleanupPrivateVars(builder, moduleTranslation, opInst.getLoc(),
2419-
privateVarsInfo.llvmPrivateVars,
2420-
privateVarsInfo.privateDecls)))
2415+
privateVarsInfo.llvmVars,
2416+
privateVarsInfo.privatizers)))
24212417
return llvm::make_error<PreviouslyReportedError>();
24222418

24232419
builder.restoreIP(oldIP);
@@ -2544,8 +2540,8 @@ convertOmpSimd(Operation &opInst, llvm::IRBuilderBase &builder,
25442540
order, simdlen, safelen);
25452541

25462542
return cleanupPrivateVars(builder, moduleTranslation, simdOp.getLoc(),
2547-
privateVarsInfo.llvmPrivateVars,
2548-
privateVarsInfo.privateDecls);
2543+
privateVarsInfo.llvmVars,
2544+
privateVarsInfo.privatizers);
25492545
}
25502546

25512547
/// Converts an OpenMP loop nest into LLVM IR using OpenMPIRBuilder.
@@ -4182,8 +4178,8 @@ convertOmpDistribute(Operation &opInst, llvm::IRBuilderBase &builder,
41824178
return llvm::make_error<PreviouslyReportedError>();
41834179

41844180
if (failed(copyFirstPrivateVars(
4185-
builder, moduleTranslation, privVarsInfo.mlirPrivateVars,
4186-
privVarsInfo.llvmPrivateVars, privVarsInfo.privateDecls)))
4181+
builder, moduleTranslation, privVarsInfo.mlirVars,
4182+
privVarsInfo.llvmVars, privVarsInfo.privatizers)))
41874183
return llvm::make_error<PreviouslyReportedError>();
41884184

41894185
llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
@@ -4224,9 +4220,9 @@ convertOmpDistribute(Operation &opInst, llvm::IRBuilderBase &builder,
42244220
return wsloopIP.takeError();
42254221
}
42264222

4227-
if (failed(cleanupPrivateVars(
4228-
builder, moduleTranslation, distributeOp.getLoc(),
4229-
privVarsInfo.llvmPrivateVars, privVarsInfo.privateDecls)))
4223+
if (failed(cleanupPrivateVars(builder, moduleTranslation,
4224+
distributeOp.getLoc(), privVarsInfo.llvmVars,
4225+
privVarsInfo.privatizers)))
42304226
return llvm::make_error<PreviouslyReportedError>();
42314227

42324228
return llvm::Error::success();
@@ -4860,7 +4856,7 @@ convertOmpTarget(Operation &opInst, llvm::IRBuilderBase &builder,
48604856
return llvm::make_error<PreviouslyReportedError>();
48614857

48624858
SmallVector<Region *> privateCleanupRegions;
4863-
llvm::transform(privateVarsInfo.privateDecls,
4859+
llvm::transform(privateVarsInfo.privatizers,
48644860
std::back_inserter(privateCleanupRegions),
48654861
[](omp::PrivateClauseOp privatizer) {
48664862
return &privatizer.getDeallocRegion();
@@ -4875,7 +4871,7 @@ convertOmpTarget(Operation &opInst, llvm::IRBuilderBase &builder,
48754871
builder.SetInsertPoint(*exitBlock);
48764872
if (!privateCleanupRegions.empty()) {
48774873
if (failed(inlineOmpRegionCleanup(
4878-
privateCleanupRegions, privateVarsInfo.llvmPrivateVars,
4874+
privateCleanupRegions, privateVarsInfo.llvmVars,
48794875
moduleTranslation, builder, "omp.targetop.private.cleanup",
48804876
/*shouldLoadCleanupRegionArg=*/false))) {
48814877
return llvm::createStringError(

0 commit comments

Comments
 (0)