Skip to content

[Flang][OpenMP] Support lowering of simd reductions #112194

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 16, 2024
Merged

[Flang][OpenMP] Support lowering of simd reductions #112194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ca87f25
[Flang][OpenMP] Support lowering of simd reductions
skatrak Oct 14, 2024
e1334f5
Address review comments
skatrak Oct 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions flang/lib/Lower/OpenMP/OpenMP.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2073,7 +2073,9 @@ static void genStandaloneSimd(lower::AbstractConverter &converter,
loopNestClauseOps, iv);

EntryBlockArgs simdArgs;
// TODO: Add private, reduction syms and vars.
// TODO: Add private syms and vars.
simdArgs.reduction.syms = simdReductionSyms;
simdArgs.reduction.vars = simdClauseOps.reductionVars;
auto simdOp =
genWrapperOp<mlir::omp::SimdOp>(converter, loc, simdClauseOps, simdArgs);

Expand Down Expand Up @@ -2231,7 +2233,9 @@ static void genCompositeDistributeParallelDoSimd(
wsloopOp.setComposite(/*val=*/true);

EntryBlockArgs simdArgs;
// TODO: Add private, reduction syms and vars.
// TODO: Add private syms and vars.
simdArgs.reduction.syms = simdReductionSyms;
simdArgs.reduction.vars = simdClauseOps.reductionVars;
auto simdOp =
genWrapperOp<mlir::omp::SimdOp>(converter, loc, simdClauseOps, simdArgs);
simdOp.setComposite(/*val=*/true);
Expand Down Expand Up @@ -2288,7 +2292,9 @@ static void genCompositeDistributeSimd(lower::AbstractConverter &converter,
distributeOp.setComposite(/*val=*/true);

EntryBlockArgs simdArgs;
// TODO: Add private, reduction syms and vars.
// TODO: Add private syms and vars.
simdArgs.reduction.syms = simdReductionSyms;
simdArgs.reduction.vars = simdClauseOps.reductionVars;
auto simdOp =
genWrapperOp<mlir::omp::SimdOp>(converter, loc, simdClauseOps, simdArgs);
simdOp.setComposite(/*val=*/true);
Expand Down Expand Up @@ -2345,7 +2351,9 @@ static void genCompositeDoSimd(lower::AbstractConverter &converter,
wsloopOp.setComposite(/*val=*/true);

EntryBlockArgs simdArgs;
// TODO: Add private, reduction syms and vars.
// TODO: Add private syms and vars.
simdArgs.reduction.syms = simdReductionSyms;
simdArgs.reduction.vars = simdClauseOps.reductionVars;
auto simdOp =
genWrapperOp<mlir::omp::SimdOp>(converter, loc, simdClauseOps, simdArgs);
simdOp.setComposite(/*val=*/true);
Expand Down
24 changes: 24 additions & 0 deletions flang/test/Lower/OpenMP/simd.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
! RUN: %flang_fc1 -flang-experimental-hlfir -emit-hlfir -fopenmp -fopenmp-version=50 %s -o - | FileCheck %s
! RUN: bbc -hlfir -emit-hlfir -fopenmp -fopenmp-version=50 %s -o - | FileCheck %s

!CHECK: omp.declare_reduction @[[REDUCER:.*]] : i32

!CHECK-LABEL: func @_QPsimd()
subroutine simd
integer :: i
Expand Down Expand Up @@ -274,3 +276,25 @@ subroutine lastprivate_with_simd
sum = i + 1
end do
end subroutine

!CHECK-LABEL: func @_QPsimd_with_reduction_clause()
subroutine simd_with_reduction_clause
integer :: i, x
x = 0
! CHECK: %[[LB:.*]] = arith.constant 1 : i32
! CHECK-NEXT: %[[UB:.*]] = arith.constant 9 : i32
! CHECK-NEXT: %[[STEP:.*]] = arith.constant 1 : i32
! CHECK-NEXT: omp.simd reduction(@[[REDUCER]] %[[X:.*]]#0 -> %[[X_RED:.*]] : !fir.ref<i32>) {
! CHECK-NEXT: omp.loop_nest (%[[I:.*]]) : i32 = (%[[LB]]) to (%[[UB]]) inclusive step (%[[STEP]]) {
!$omp simd reduction(+:x)
do i=1, 9
! CHECK: %[[X_DECL:.*]]:2 = hlfir.declare %[[X_RED]] {uniq_name = "_QFsimd_with_reduction_clauseEx"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
! CHECK: fir.store %[[I]] to %[[LOCAL:.*]]#1 : !fir.ref<i32>
Copy link
Contributor

@tblah tblah Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not this PR, but reading the full IR locally it looks like the loop iteration variable was not privatized.

As I understand it, OpenMP 5.2 section 5.1.1 line 16 seems to say that it should be implicitly lastprivate.

Did I miss something here? If not I'll create an issue.

Copy link
Member Author

@skatrak skatrak Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, I think the previous statement in the spec is the one that applies, since it has a single associated loop. So I guess it should actually be linear with a linear-step equal to the increment of the loop. But I believe you're right, since neither that nor lastprivatization in the case of multiple associated loops seem to be currently implemented.

! CHECK: %[[X_LD:.*]] = fir.load %[[X_DECL]]#0 : !fir.ref<i32>
! CHECK: %[[I_LD:.*]] = fir.load %[[LOCAL]]#0 : !fir.ref<i32>
! CHECK: %[[SUM:.*]] = arith.addi %[[X_LD]], %[[I_LD]] : i32
! CHECK: hlfir.assign %[[SUM]] to %[[X_DECL]]#0 : i32, !fir.ref<i32>
x = x+i
end do
!$OMP end simd
end subroutine
8 changes: 5 additions & 3 deletions mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2016,14 +2016,16 @@ void SimdOp::build(OpBuilder &builder, OperationState &state,
const SimdOperands &clauses) {
MLIRContext *ctx = builder.getContext();
// TODO Store clauses in op: linearVars, linearStepVars, privateVars,
// privateSyms, reductionVars, reductionByref, reductionSyms.
// privateSyms.
SimdOp::build(builder, state, clauses.alignedVars,
makeArrayAttr(ctx, clauses.alignments), clauses.ifExpr,
/*linear_vars=*/{}, /*linear_step_vars=*/{},
clauses.nontemporalVars, clauses.order, clauses.orderMod,
/*private_vars=*/{}, /*private_syms=*/nullptr,
/*reduction_vars=*/{}, /*reduction_byref=*/nullptr,
/*reduction_syms=*/nullptr, clauses.safelen, clauses.simdlen);
clauses.reductionVars,
makeDenseBoolArrayAttr(ctx, clauses.reductionByref),
makeArrayAttr(ctx, clauses.reductionSyms), clauses.safelen,
clauses.simdlen);
}

LogicalResult SimdOp::verify() {
Expand Down
21 changes: 16 additions & 5 deletions mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1786,18 +1786,29 @@ convertOrderKind(std::optional<omp::ClauseOrderKind> o) {
llvm_unreachable("Unknown ClauseOrderKind kind");
}

static LogicalResult simdOpSupported(omp::SimdOp op) {
if (!op.getLinearVars().empty() || !op.getLinearStepVars().empty())
return op.emitError("linear clause not yet supported");

if (!op.getPrivateVars().empty() || op.getPrivateSyms())
return op.emitError("privatization clauses not yet supported");

if (!op.getReductionVars().empty() || op.getReductionByref() ||
op.getReductionSyms())
return op.emitError("reduction clause not yet supported");

return success();
}

/// Converts an OpenMP simd loop into LLVM IR using OpenMPIRBuilder.
static LogicalResult
convertOmpSimd(Operation &opInst, llvm::IRBuilderBase &builder,
LLVM::ModuleTranslation &moduleTranslation) {
auto simdOp = cast<omp::SimdOp>(opInst);
auto loopOp = cast<omp::LoopNestOp>(simdOp.getWrappedLoop());

if (!simdOp.getLinearVars().empty() || !simdOp.getLinearStepVars().empty() ||
!simdOp.getPrivateVars().empty() || simdOp.getPrivateSyms() ||
!simdOp.getReductionVars().empty() || simdOp.getReductionByref() ||
simdOp.getReductionSyms())
return opInst.emitError("unhandled clauses for translation to LLVM IR");
if (failed(simdOpSupported(simdOp)))
return failure();

llvm::OpenMPIRBuilder::LocationDescription ompLoc(builder);

Expand Down
Loading