Skip to content

Commit 56c1105

Browse files
anchurajanchuraj
committed
R6: Addressing review comments
1 parent 2adadc1 commit 56c1105

File tree

5 files changed

+54
-31
lines changed

5 files changed

+54
-31
lines changed

‎mlir/include/mlir/Dialect/OpenMP/OpenMPEnums.td‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -184,14 +184,14 @@ def OrderModifierAttr : EnumAttr<OpenMP_Dialect, OrderModifier,
184184
//===----------------------------------------------------------------------===//
185185

186186
def ReductionModifierDefault : I32EnumAttrCase<"defaultmod", 0>;
187-
def ReductionModifierInScan : I32EnumAttrCase<"inscan", 1>;
187+
def ReductionModifierInscan : I32EnumAttrCase<"inscan", 1>;
188188
def ReductionModifierTask : I32EnumAttrCase<"task", 2>;
189189

190190
def ReductionModifier : OpenMP_I32EnumAttr<
191191
"ReductionModifier",
192192
"reduction modifier", [
193193
ReductionModifierDefault,
194-
ReductionModifierInScan,
194+
ReductionModifierInscan,
195195
ReductionModifierTask
196196
]>;
197197

‎mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp‎

Lines changed: 12 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -835,9 +835,9 @@ struct ReductionPrintArgs {
835835
TypeRange types;
836836
DenseBoolArrayAttr byref;
837837
ArrayAttr syms;
838-
ReductionModifierAttr *modifier;
838+
ReductionModifierAttr modifier;
839839
ReductionPrintArgs(ValueRange vars, TypeRange types, DenseBoolArrayAttr byref,
840-
ArrayAttr syms, ReductionModifierAttr *mod = nullptr)
840+
ArrayAttr syms, ReductionModifierAttr mod = nullptr)
841841
: vars(vars), types(types), byref(byref), syms(syms), modifier(mod) {}
842842
};
843843
struct AllRegionPrintArgs {
@@ -857,14 +857,14 @@ static void printClauseWithRegionArgs(
857857
ValueRange argsSubrange, ValueRange operands, TypeRange types,
858858
ArrayAttr symbols = nullptr, DenseI64ArrayAttr mapIndices = nullptr,
859859
DenseBoolArrayAttr byref = nullptr,
860-
ReductionModifierAttr *modifier = nullptr) {
860+
ReductionModifierAttr modifier = nullptr) {
861861
if (argsSubrange.empty())
862862
return;
863863

864864
p << clauseName << "(";
865865

866-
if (modifier && *modifier)
867-
p << "mod: " << stringifyReductionModifier(modifier->getValue()) << ", ";
866+
if (modifier)
867+
p << "mod: " << stringifyReductionModifier(modifier.getValue()) << ", ";
868868

869869
if (!symbols) {
870870
llvm::SmallVector<Attribute> values(operands.size(), nullptr);
@@ -998,7 +998,7 @@ static void printInReductionPrivateReductionRegion(
998998
args.privateArgs.emplace(privateVars, privateTypes, privateSyms,
999999
/*mapIndices=*/nullptr);
10001000
args.reductionArgs.emplace(reductionVars, reductionTypes, reductionByref,
1001-
reductionSyms, &reductionMod);
1001+
reductionSyms, reductionMod);
10021002
printBlockArgRegion(p, op, region, args);
10031003
}
10041004

@@ -1021,7 +1021,7 @@ static void printPrivateReductionRegion(
10211021
args.privateArgs.emplace(privateVars, privateTypes, privateSyms,
10221022
/*mapIndices=*/nullptr);
10231023
args.reductionArgs.emplace(reductionVars, reductionTypes, reductionByref,
1024-
reductionSyms, &reductionMod);
1024+
reductionSyms, reductionMod);
10251025
printBlockArgRegion(p, op, region, args);
10261026
}
10271027

@@ -3163,26 +3163,19 @@ void ScanOp::build(OpBuilder &builder, OperationState &state,
31633163
}
31643164

31653165
LogicalResult ScanOp::verify() {
3166-
if (hasExclusiveVars() && hasInclusiveVars()) {
3166+
if (hasExclusiveVars() == hasInclusiveVars())
31673167
return emitError(
31683168
"Exactly one of EXCLUSIVE or INCLUSIVE clause is expected");
3169-
}
3170-
if (mlir::omp::WsloopOp parentWsLoopOp =
3171-
(*this)->getParentOfType<mlir::omp::WsloopOp>()) {
3169+
if (WsloopOp parentWsLoopOp = (*this)->getParentOfType<WsloopOp>())
31723170
if (parentWsLoopOp.getReductionModAttr() &&
31733171
parentWsLoopOp.getReductionModAttr().getValue() ==
3174-
mlir::omp::ReductionModifier::inscan) {
3172+
mlir::omp::ReductionModifier::inscan)
31753173
return success();
3176-
}
3177-
}
3178-
if (mlir::omp::SimdOp parentSimdOp =
3179-
(*this)->getParentOfType<mlir::omp::SimdOp>()) {
3174+
if (SimdOp parentSimdOp = (*this)->getParentOfType<SimdOp>())
31803175
if (parentSimdOp.getReductionModAttr() &&
31813176
parentSimdOp.getReductionModAttr().getValue() ==
3182-
mlir::omp::ReductionModifier::inscan) {
3177+
mlir::omp::ReductionModifier::inscan)
31833178
return success();
3184-
}
3185-
}
31863179
return emitError("SCAN directive needs to be enclosed within a parent "
31873180
"worksharing loop construct or SIMD construct with INSCAN "
31883181
"reduction modifier");

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

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -241,9 +241,13 @@ static LogicalResult checkImplementationStatus(Operation &op) {
241241
}
242242
};
243243
auto checkReduction = [&todo](auto op, LogicalResult &result) {
244-
if (!op.getReductionVars().empty() || op.getReductionByref() ||
245-
op.getReductionSyms())
246-
result = todo("reduction");
244+
if (isa<omp::TeamsOp>(op) || isa<omp::SimdOp>(op))
245+
if (!op.getReductionVars().empty() || op.getReductionByref() ||
246+
op.getReductionSyms())
247+
result = todo("reduction");
248+
if (op.getReductionMod() &&
249+
op.getReductionMod().value() != omp::ReductionModifier::defaultmod)
250+
result = todo("reduction with modifier");
247251
};
248252
auto checkTaskReduction = [&todo](auto op, LogicalResult &result) {
249253
if (!op.getTaskReductionVars().empty() || op.getTaskReductionByref() ||
@@ -261,6 +265,7 @@ static LogicalResult checkImplementationStatus(Operation &op) {
261265
.Case([&](omp::SectionsOp op) {
262266
checkAllocate(op, result);
263267
checkPrivate(op, result);
268+
checkReduction(op, result);
264269
})
265270
.Case([&](omp::SingleOp op) {
266271
checkAllocate(op, result);
@@ -289,8 +294,12 @@ static LogicalResult checkImplementationStatus(Operation &op) {
289294
checkAllocate(op, result);
290295
checkLinear(op, result);
291296
checkOrder(op, result);
297+
checkReduction(op, result);
298+
})
299+
.Case([&](omp::ParallelOp op) {
300+
checkAllocate(op, result);
301+
checkReduction(op, result);
292302
})
293-
.Case([&](omp::ParallelOp op) { checkAllocate(op, result); })
294303
.Case([&](omp::SimdOp op) {
295304
checkLinear(op, result);
296305
checkNontemporal(op, result);
@@ -4591,10 +4600,6 @@ convertHostOrTargetOperation(Operation *op, llvm::IRBuilderBase &builder,
45914600
.Case([&](omp::AtomicCaptureOp op) {
45924601
return convertOmpAtomicCapture(op, builder, moduleTranslation);
45934602
})
4594-
.Case([&](omp::ScanOp) {
4595-
return op->emitError()
4596-
<< "not yet implemented: " << op->getName() << " operation";
4597-
})
45984603
.Case([&](omp::SectionsOp) {
45994604
return convertOmpSections(*op, builder, moduleTranslation);
46004605
})

‎mlir/test/Dialect/OpenMP/invalid.mlir‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1837,6 +1837,32 @@ combiner {
18371837
omp.yield (%1 : f32)
18381838
}
18391839

1840+
func.func @scan_test_2(%lb: i32, %ub: i32, %step: i32) {
1841+
%test1f32 = "test.f32"() : () -> (!llvm.ptr)
1842+
omp.wsloop reduction(mod:inscan, @add_f32 %test1f32 -> %arg1 : !llvm.ptr) {
1843+
omp.loop_nest (%i, %j) : i32 = (%lb, %ub) to (%ub, %lb) step (%step, %step) {
1844+
// expected-error @below {{Exactly one of EXCLUSIVE or INCLUSIVE clause is expected}}
1845+
omp.scan
1846+
omp.yield
1847+
}
1848+
}
1849+
return
1850+
}
1851+
1852+
// -----
1853+
1854+
omp.declare_reduction @add_f32 : f32
1855+
init {
1856+
^bb0(%arg: f32):
1857+
%0 = arith.constant 0.0 : f32
1858+
omp.yield (%0 : f32)
1859+
}
1860+
combiner {
1861+
^bb1(%arg0: f32, %arg1: f32):
1862+
%1 = arith.addf %arg0, %arg1 : f32
1863+
omp.yield (%1 : f32)
1864+
}
1865+
18401866
func.func @scan_test_2(%lb: i32, %ub: i32, %step: i32) {
18411867
%test1f32 = "test.f32"() : () -> (!llvm.ptr)
18421868
omp.wsloop reduction(mod:inscan, @add_f32 %test1f32 -> %arg1 : !llvm.ptr) {

‎mlir/test/Target/LLVMIR/openmp-todo.mlir‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -204,11 +204,10 @@ atomic {
204204
omp.yield
205205
}
206206
llvm.func @scan_reduction(%lb : i32, %ub : i32, %step : i32, %x : !llvm.ptr) {
207+
// expected-error@below {{not yet implemented: Unhandled clause reduction with modifier in omp.wsloop operation}}
207208
// expected-error@below {{LLVM Translation failed for operation: omp.wsloop}}
208209
omp.wsloop reduction(mod:inscan, @add_f32 %x -> %prv : !llvm.ptr) {
209210
omp.loop_nest (%iv) : i32 = (%lb) to (%ub) step (%step) {
210-
// expected-error@below {{not yet implemented: omp.scan operation}}
211-
// expected-error@below {{LLVM Translation failed for operation: omp.scan}}
212211
omp.scan inclusive(%prv : !llvm.ptr)
213212
omp.yield
214213
}

0 commit comments

Comments
 (0)