Skip to content

Commit 51f3713

Browse files
committed
[mlir][LLVMIR][OpenMP] fix dominance for reduction init block
It was incorrect to set the insertion point to the init block after inlining the initialization region because the code generated in the init block depends upon the value yielded from the init region. When there were multiple reduction initialization regions each with multiple blocks, this could lead to the initilization region being inlined after the init block which depends upon it. Moving the insertion point to before inlining the initialization block turned up further issues around the handling of the terminator for the initialization block, which are also fixed here. This fixes a bug in llvm#92430 (but the affected code couldn't compile before llvm#92430 anyway).
1 parent 6467b49 commit 51f3713

File tree

3 files changed

+361
-4
lines changed

3 files changed

+361
-4
lines changed

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

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -388,8 +388,18 @@ static LogicalResult inlineConvertOmpRegions(
388388
// be processed multiple times.
389389
moduleTranslation.forgetMapping(region);
390390

391-
if (potentialTerminator && potentialTerminator->isTerminator())
392-
potentialTerminator->insertAfter(&builder.GetInsertBlock()->back());
391+
if (potentialTerminator && potentialTerminator->isTerminator()) {
392+
llvm::BasicBlock *block = builder.GetInsertBlock();
393+
if (block->empty())
394+
// this can happen for really simple reduction init regions e.g.
395+
// %0 = llvm.mlir.constant(0 : i32) : i32
396+
// omp.yield(%0 : i32)
397+
// because the llvm.mlir.constant (MLIR op) isn't converted into any
398+
// llvm op
399+
potentialTerminator->insertInto(block, block->begin());
400+
else
401+
potentialTerminator->insertAfter(&block->back());
402+
}
393403

394404
return success();
395405
}
@@ -1171,6 +1181,8 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
11711181
}
11721182
}
11731183

1184+
builder.SetInsertPoint(initBlock->getFirstNonPHIOrDbgOrAlloca());
1185+
11741186
for (unsigned i = 0; i < opInst.getNumReductionVars(); ++i) {
11751187
SmallVector<llvm::Value *> phis;
11761188

@@ -1183,7 +1195,10 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
11831195
assert(phis.size() == 1 &&
11841196
"expected one value to be yielded from the "
11851197
"reduction neutral element declaration region");
1186-
builder.SetInsertPoint(initBlock->getTerminator());
1198+
1199+
// mapInitializationArg finishes its block with a terminator. We need to
1200+
// insert before that terminator.
1201+
builder.SetInsertPoint(builder.GetInsertBlock()->getTerminator());
11871202

11881203
if (isByRef[i]) {
11891204
// Store the result of the inlined region to the allocated reduction var

0 commit comments

Comments
 (0)