llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-openacc Author: Kareem Ergawy (ergawy) <details> <summary>Changes</summary> Such a loop was classified separately by a previous change but still lowered as a raw CFG, so its structured form was lost. Lower it structurally instead, with its body folded into a region that can hold the branching. The loop keeps its bounds on the op, so it remains available to whatever transforms or parallelizes it. Only the body is folded: the loop control statements are emitted as they are for any structured loop, since a branch from outside may target either of them. Loops an OpenACC or OpenMP directive owns are lowered the same way, so they keep their form too. --- Patch is 43.56 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/225758.diff 16 Files Affected: - (modified) flang/include/flang/Lower/AbstractConverter.h (+8) - (modified) flang/include/flang/Lower/PFTBuilder.h (+10-6) - (modified) flang/lib/Lower/Bridge.cpp (+119-11) - (modified) flang/lib/Lower/OpenMP/OpenMP.cpp (+8) - (modified) flang/lib/Lower/PFTBuilder.cpp (+4) - (modified) flang/test/Lower/OpenACC/acc-cache.f90 (+9-10) - (added) flang/test/Lower/OpenACC/acc-unstructured-internals.f90 (+73) - (modified) flang/test/Lower/OpenACC/acc-unstructured.f90 (+8-9) - (removed) flang/test/Lower/OpenMP/Todo/metadirective-loop-unstructured.f90 (-39) - (added) flang/test/Lower/OpenMP/metadirective-loop-unstructured.f90 (+50) - (modified) flang/test/Lower/OpenMP/wsloop-unstructured-cycle.f90 (+34-28) - (added) flang/test/Lower/OpenMP/wsloop-unstructured-internals.f90 (+44) - (modified) flang/test/Lower/always-execute-loop-body.f90 (+6-2) - (added) flang/test/Lower/do-loop-branch-to-loop-header.f90 (+55) - (modified) flang/test/Lower/do_loop_unstructured.f90 (+20-134) - (modified) flang/test/Lower/select-case-statement.f90 (+5-2) ``````````diff diff --git a/flang/include/flang/Lower/AbstractConverter.h b/flang/include/flang/Lower/AbstractConverter.h index ae246d3188bd88..40d94dc9f5bfdd 100644 --- a/flang/include/flang/Lower/AbstractConverter.h +++ b/flang/include/flang/Lower/AbstractConverter.h @@ -384,6 +384,14 @@ class AbstractConverter { virtual void genEval(pft::Evaluation &eval, bool unstructuredContext = true) = 0; + /// Emit \p loopEval's evaluations, folding the body into an + /// scf.execute_region when the loop's branching is confined to that body. + /// The loop control statements are emitted outside any wrap, exactly as they + /// are for a structured loop. Used by directive lowering, which consumes the + /// DO itself and so never reaches genFIR(DoConstruct), where a plain loop's + /// body is wrapped. + virtual void genLoopBodyEvaluations(pft::Evaluation &loopEval) = 0; + /// Return options controlling lowering behavior. const Fortran::lower::LoweringOptions &getLoweringOptions() const { return loweringOptions; diff --git a/flang/include/flang/Lower/PFTBuilder.h b/flang/include/flang/Lower/PFTBuilder.h index 29f0573eb57e4d..b30f20da204232 100644 --- a/flang/include/flang/Lower/PFTBuilder.h +++ b/flang/include/flang/Lower/PFTBuilder.h @@ -356,17 +356,21 @@ struct Evaluation : EvaluationVariant { controlFlow = std::min(controlFlow, kind); } - /// True when control flow is not fully structured, category (c) included. - /// Existing consumers ask this to decide whether raw blocks are needed, and - /// a category (c) construct still needs them until its body is wrapped, so - /// it must answer true here. Use hasUnstructuredInternals() to single out - /// category (c) itself. - bool isUnstructured() const { return controlFlow != ControlFlow::Structured; } + /// True only for fully unstructured control flow, which is lowered as raw + /// CFG blocks. + bool isUnstructured() const { + return controlFlow == ControlFlow::Unstructured; + } bool hasUnstructuredInternals() const { return controlFlow == ControlFlow::StructuredWithUnstructuredInternals; } + /// True when this construct is lowered structurally, yet its body holds + /// unstructured control flow that has to be folded into an + /// scf.execute_region. + bool lowerBodyAsWrappedRegion() const; + bool lowerAsStructured() const; bool lowerAsUnstructured() const; bool forceAsUnstructured() const; diff --git a/flang/lib/Lower/Bridge.cpp b/flang/lib/Lower/Bridge.cpp index 784a87cc619979..ed862d6b3374de 100644 --- a/flang/lib/Lower/Bridge.cpp +++ b/flang/lib/Lower/Bridge.cpp @@ -1125,6 +1125,11 @@ class FirConverter : public Fortran::lower::AbstractConverter { genFIR(eval, unstructuredContext); } + void genLoopBodyEvaluations( + Fortran::lower::pft::Evaluation &loopEval) override final { + genLoopBodyEvaluations(loopEval, /*unstructuredContext=*/true); + } + //===--------------------------------------------------------------------===// // Utility methods //===--------------------------------------------------------------------===// @@ -2637,6 +2642,97 @@ class FirConverter : public Fortran::lower::AbstractConverter { return wrapOp; } + /// Wrap a loop's *body* -- not the construct, and not the loop control -- in + /// an scf.execute_region. A category (c) loop keeps its structured loop op + /// while its self-contained raw branching lives inside the region, which may + /// hold as many blocks as it needs. + /// + /// The builder must already be positioned inside the loop body. On return it + /// is inside the region. Returns null when the loop has no such internals. + mlir::scf::ExecuteRegionOp + wrapUnstructuredBody(Fortran::lower::pft::Evaluation &eval, + mlir::Block *&yieldBlock, + mlir::Block *&savedEndDoBlock) { + if (!eval.lowerBodyAsWrappedRegion()) + return nullptr; + + Fortran::lower::pft::EvaluationList &list = eval.getNestedEvaluations(); + mlir::Location loc = toLocation(); + auto wrapOp = + mlir::scf::ExecuteRegionOp::create(*builder, loc, mlir::TypeRange{}, + /*noInline=*/builder->getUnitAttr()); + ++wrapUnstructuredCount; + mlir::Block *entry = builder->createBlock(&wrapOp.getRegion()); + builder->setInsertionPointToEnd(entry); + // Only the body: both loop control statements live in the enclosing region + // and may be branched to from outside the loop, so neither may take its + // block from this region. Dropping only the EndDoStmt leaves the DO + // statement to be given a block here, which any GOTO targeting the loop + // head would then reference across a region boundary. + createEmptyBlocksIn( + llvm::make_range(std::next(list.begin()), std::prev(list.end()))); + yieldBlock = builder->createBlock(&wrapOp.getRegion()); + builder->setInsertionPointToEnd(yieldBlock); + mlir::scf::YieldOp::create(*builder, loc); + + // A CYCLE targets the EndDoStmt, which is the boundary between the loop + // body and the loop control. Inside the wrap that boundary is the region's + // yield block, so a CYCLE leaves the region rather than branching to a + // block the region cannot name. + savedEndDoBlock = list.back().block; + list.back().block = yieldBlock; + + builder->setInsertionPointToEnd(entry); + return wrapOp; + } + + /// Emit a loop's evaluations, optionally folding the body into an + /// scf.execute_region. The loop control statements -- the first and last + /// evaluations -- are emitted exactly as they are for a structured loop: + /// same context flag, same region, same blocks. They may be branch targets + /// from outside the loop, so their blocks have to stay in the enclosing + /// region. Only what lies strictly between them goes inside the wrap. + void genLoopBodyEvaluations(Fortran::lower::pft::Evaluation &eval, + bool unstructuredContext) { + Fortran::lower::pft::EvaluationList &list = eval.getNestedEvaluations(); + auto iter = list.begin(); + auto end = std::prev(list.end()); + + // The loop control statement, outside any wrap. + if (iter != end) { + genFIR(*iter, unstructuredContext); + ++iter; + } + + mlir::Block *yieldBlock = nullptr; + mlir::Block *savedEndDoBlock = nullptr; + mlir::scf::ExecuteRegionOp wrapOp = + wrapUnstructuredBody(eval, yieldBlock, savedEndDoBlock); + for (; iter != end; ++iter) + genFIR(*iter, unstructuredContext || wrapOp); + closeUnstructuredBodyWrap(wrapOp, eval, yieldBlock, savedEndDoBlock); + } + + /// Finalize a wrap created by wrapUnstructuredBody: restore the EndDoStmt + /// block, fall through to the region's yield, and resume after the wrap op + /// so the caller emits the loop control outside it. + void closeUnstructuredBodyWrap(mlir::scf::ExecuteRegionOp wrapOp, + Fortran::lower::pft::Evaluation &eval, + mlir::Block *yieldBlock, + mlir::Block *savedEndDoBlock) { + if (!wrapOp) + return; + + eval.getNestedEvaluations().back().block = savedEndDoBlock; + + if (mlir::Block *current = builder->getBlock()) + if (current->empty() || + !current->back().hasTrait<mlir::OpTrait::IsTerminator>()) + genBranch(yieldBlock); + + builder->setInsertionPointAfter(wrapOp); + } + /// Finalize a wrap created by wrapUnstructuredConstruct: restore the /// original exit block and set the insertion point after the wrap op. void closeUnstructuredWrap(mlir::scf::ExecuteRegionOp wrapOp, @@ -2664,9 +2760,10 @@ class FirConverter : public Fortran::lower::AbstractConverter { // skip generating any loop — just lower the body. The IV value is // already available from the parent acc.loop's block argument. if (Fortran::lower::isCollapsedDoConstruct(doConstruct)) { - auto iter = eval.getNestedEvaluations().begin(); - for (auto end = --eval.getNestedEvaluations().end(); iter != end; ++iter) - genFIR(*iter, unstructuredContext); + // The parent acc.loop supplies the iteration, so no loop op is built + // here for a wrap to sit inside. The body may still hold self-contained + // raw branching, so wrap it directly. + genLoopBodyEvaluations(eval, unstructuredContext); return; } @@ -2689,11 +2786,10 @@ class FirConverter : public Fortran::lower::AbstractConverter { builder->getInsertionPoint()->getBlock()->getParent()) && "builder insertion point is not inside the newly generated loop"); - // Loop body code. - auto iter = eval.getNestedEvaluations().begin(); - for (auto end = --eval.getNestedEvaluations().end(); iter != end; - ++iter) - genFIR(*iter, unstructuredContext); + // The acc.loop supplies the iteration, so the body is emitted here + // rather than through the increment-loop path below. Wrap it when it + // holds self-contained raw branching. + genLoopBodyEvaluations(eval, unstructuredContext); builder->setInsertionPointAfter(loopOp); return; @@ -2842,10 +2938,12 @@ class FirConverter : public Fortran::lower::AbstractConverter { if (!infiniteLoop && !whileCondition) genFIRIncrementLoopBegin(incrementLoopNestInfo, doStmtEval.dirs); + // The loop control is structured, but the body may hold raw branching + // confined to it. Wrap the body, leaving the loop control outside, so the + // structured loop op's single-block region stays well formed. // Loop body code. - auto iter = eval.getNestedEvaluations().begin(); - for (auto end = --eval.getNestedEvaluations().end(); iter != end; ++iter) - genFIR(*iter, unstructuredContext); + genLoopBodyEvaluations(eval, unstructuredContext); + auto iter = std::prev(eval.getNestedEvaluations().end()); // An EndDoStmt in unstructured code may start a new block. Fortran::lower::pft::Evaluation &endDoEval = *iter; @@ -6469,6 +6567,16 @@ class FirConverter : public Fortran::lower::AbstractConverter { /// boundaries. void createEmptyBlocks( std::list<Fortran::lower::pft::Evaluation> &evaluationList) { + createEmptyBlocksIn( + llvm::make_range(evaluationList.begin(), evaluationList.end())); + } + + /// createEmptyBlocks over a sub-range of an evaluation list. A body-only wrap + /// must not pre-create blocks for the loop control statements, which live in + /// the enclosing region. + void createEmptyBlocksIn( + llvm::iterator_range<Fortran::lower::pft::EvaluationList::iterator> + evaluationList) { mlir::Region *region = &builder->getRegion(); for (Fortran::lower::pft::Evaluation &eval : evaluationList) { if (eval.isNewBlock) diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp index df14d13b76a4bd..edbb14a745a55a 100644 --- a/flang/lib/Lower/OpenMP/OpenMP.cpp +++ b/flang/lib/Lower/OpenMP/OpenMP.cpp @@ -1018,6 +1018,14 @@ static void genNestedEvaluations(lower::AbstractConverter &converter, int collapseValue = 0) { lower::pft::Evaluation *curEval = getCollapsedLoopEval(eval, collapseValue); + // The directive consumes the DO itself, so genFIR(DoConstruct) -- where a + // plain loop's body is wrapped -- is never reached. Go through the same + // helper here, so the loop control statements are emitted outside the wrap + // exactly as they are for a structured loop. + if (curEval->isA<parser::DoConstruct>() && curEval->hasNestedEvaluations()) { + converter.genLoopBodyEvaluations(*curEval); + return; + } for (lower::pft::Evaluation &e : curEval->getNestedEvaluations()) converter.genEval(e); } diff --git a/flang/lib/Lower/PFTBuilder.cpp b/flang/lib/Lower/PFTBuilder.cpp index 410bab33a80355..0a8d7a9dcd20af 100644 --- a/flang/lib/Lower/PFTBuilder.cpp +++ b/flang/lib/Lower/PFTBuilder.cpp @@ -1735,6 +1735,10 @@ bool Fortran::lower::pft::Evaluation::lowerAsUnstructured() const { return isUnstructured() || clDisableStructuredFir; } +bool Fortran::lower::pft::Evaluation::lowerBodyAsWrappedRegion() const { + return hasUnstructuredInternals() && !lowerAsUnstructured(); +} + bool Fortran::lower::pft::Evaluation::forceAsUnstructured() const { return clDisableStructuredFir; } diff --git a/flang/test/Lower/OpenACC/acc-cache.f90 b/flang/test/Lower/OpenACC/acc-cache.f90 index 12bdee38338705..151ef27a96ddc4 100644 --- a/flang/test/Lower/OpenACC/acc-cache.f90 +++ b/flang/test/Lower/OpenACC/acc-cache.f90 @@ -387,11 +387,11 @@ subroutine test_cache_nonunit_lb() ! For arr(10:20), startIdx = 10, element 15 has lowerbound = 15 - 10 = 5 ! CHECK: %[[C10:.*]] = arith.constant 10 : index -! Unstructured loop with SELECT CASE: acc.loop becomes unstructured -! CHECK: acc.loop private({{.*}}) { -! CHECK: cf.br ^[[HEADER:.*]] -! CHECK: ^[[HEADER]]: -! CHECK: cf.cond_br %{{.*}}, ^[[BODY:.*]], ^[[EXIT:.*]] +! The SELECT CASE branches only within the loop body, so acc.loop keeps its +! structured control and the raw blocks live in a wrap inside it. +! CHECK: acc.loop private({{.*}}) +! CHECK: scf.execute_region no_inline { +! CHECK: cf.br ^[[BODY:.*]] ! CHECK: ^[[BODY]]: ! Compute lowerbound = 15 - startIdx = 15 - 10 = 5 ! CHECK: %[[C1:.*]] = arith.constant 1 : index @@ -420,13 +420,12 @@ subroutine test_cache_nonunit_lb() ! CHECK: hlfir.designate %[[DECL]]#0 ! CHECK: hlfir.assign ! CHECK: cf.br ^[[MERGE]] -! All SELECT CASE branches converge, then loop back or exit +! All SELECT CASE branches converge, then leave the wrap. The loop control is +! acc.loop's own, so the region ends at its yield rather than a back edge. ! CHECK: ^[[MERGE]]: -! CHECK: cf.br ^[[HEADER]] +! CHECK: cf.br ^[[EXIT:.*]] ! CHECK: ^[[EXIT]]: -! Scope termination: acc.yield marks end of cache scope -! CHECK: acc.yield -! CHECK-NEXT: } {{.*}}unstructured{{.*}} +! CHECK: scf.yield end subroutine ! CHECK-LABEL: func.func @_QPtest_cache_use_after_region() diff --git a/flang/test/Lower/OpenACC/acc-unstructured-internals.f90 b/flang/test/Lower/OpenACC/acc-unstructured-internals.f90 new file mode 100644 index 00000000000000..f0be621384a6ac --- /dev/null +++ b/flang/test/Lower/OpenACC/acc-unstructured-internals.f90 @@ -0,0 +1,73 @@ +! RUN: bbc -fopenacc -emit-hlfir -o - %s | FileCheck %s + +! Loops under an OpenACC directive whose control flow is structured on the +! outside but whose branching is confined to the body. The directive's own +! code-gen consumes the DO, so the body is wrapped at the directive's body +! lowering site rather than in genFIR(DoConstruct). The loop keeps its bounds +! on the op -- control(...) rather than a cf trip-count test -- so it is still +! available to be parallelized. + +! A forward GOTO raised inside a nested IF, jumping over a whole inner DO and +! landing on the last statement of the outer loop body. Both endpoints are +! inside the body, so the outer loop stays an acc.loop with control(...) and +! the two inner loops stay acc.loops of their own. +subroutine kernels_goto_over_inner(qfx, a, its, ite, jts, jte, force, flux) + real :: qfx(ite,jte), a(ite,jte) + logical :: force + integer :: flux + !$acc kernels + do j = jts, jte + do 330 i = its, ite + a(i,j) = a(i,j) + 1.0 +330 continue + if (force) then + if (flux .eq. 1) goto 350 + endif + do i = its, ite + qfx(i,j) = 0. + enddo +350 continue + enddo + !$acc end kernels +end subroutine + +! CHECK-LABEL: func.func @_QPkernels_goto_over_inner +! CHECK: acc.kernels { +! CHECK: acc.loop private({{.*}}) control(%{{.*}} : i32) = (%{{.*}} : i32) to (%{{.*}} : i32) step (%{{.*}} : i32) { +! CHECK: scf.execute_region no_inline { +! The inner loops are untouched by the wrap around the outer body. +! CHECK: acc.loop private({{.*}}) control(%{{.*}} : i32) = (%{{.*}} : i32) to (%{{.*}} : i32) step (%{{.*}} : i32) { +! CHECK: acc.yield +! The GOTO and the branches it feeds stay inside the region. +! CHECK: cf.cond_br %{{[0-9]+}}, ^bb[[THEN:[0-9]+]], ^bb[[SKIP:[0-9]+]] +! CHECK: acc.loop private({{.*}}) control(%{{.*}} : i32) = (%{{.*}} : i32) to (%{{.*}} : i32) step (%{{.*}} : i32) { +! CHECK: acc.yield +! CHECK: scf.yield +! CHECK: acc.yield + +! A CYCLE targets the EndDoStmt, the boundary between the loop body and the +! loop control, so inside the wrap it leaves the region at its yield. +subroutine parallel_loop_cycle(a, n) + real :: a(n) + !$acc parallel loop + do i = 1, n + if (a(i) > 0.0) then + a(i) = 1.0 + cycle + end if + a(i) = 2.0 + end do +end subroutine + +! CHECK-LABEL: func.func @_QPparallel_loop_cycle +! CHECK: acc.parallel combined(loop) { +! CHECK: acc.loop private({{.*}}) control(%{{.*}} : i32) = (%{{.*}} : i32) to (%{{.*}} : i32) step (%{{.*}} : i32) { +! CHECK: scf.execute_region no_inline { +! CHECK: cf.cond_br %{{[0-9]+}}, ^bb[[CYCLE:[0-9]+]], ^bb[[BODY:[0-9]+]] +! CHECK: ^bb[[CYCLE]]: +! CHECK: cf.br ^bb[[EXIT:[0-9]+]] +! CHECK: ^bb[[BODY]]: +! CHECK: cf.br ^bb[[EXIT]] +! CHECK: ^bb[[EXIT]]: +! CHECK: scf.yield +! CHECK: acc.yield diff --git a/flang/test/Lower/OpenACC/acc-unstructured.f90 b/flang/test/Lower/OpenACC/acc-unstructured.f90 index cbb27d74cc96c2..6a95f664f78dfd 100644 --- a/flang/test/Lower/OpenACC/acc-unstructured.f90 +++ b/flang/test/Lower/OpenACC/acc-unstructured.f90 @@ -275,19 +275,18 @@ subroutine test_unstructured_collapse_cycle(a) ! Both induction variables (j and i) are privatized: ! CHECK: %[[PRIVJ:.*]] = acc.private varPtr(%{{.*}} : !fir.ref<i32>) recipe(@privatization_ref_i32) implicit(true) name("j") -> !fir.ref<i32> ! CHECK: %[[PRIVI:.*]] = acc.private varPtr(%{{.*}} : !fir.ref<i32>) recipe(@privatization_ref_i32) implicit(true) name("i") -> !fir.ref<i32> -! No control(...) on acc.loop — bounds are not on the op: ! CHECK: acc.loop combined(serial) private(%[[PRIVJ]], %[[PRIVI]] : !fir.ref<i32>, !fir.ref<i32>) { -! Outer loop trip-count test (j) emitted as cf: -! CHECK: arith.cmpi sgt -! CHECK: cf.cond_br -! Inner loop trip-count test (i) emitted as cf: -! CHECK: arith.cmpi sgt -! CHECK: cf.cond_br -! The if/cycle is a structured cf branch in the body: +! The IF-guarded CYCLE branches only within the body, so both loops keep their +! bounds on the op -- control(...) rather than a cf trip-count test -- and the +! raw blocks are confined to a wrap inside each body. +! CHECK: acc.loop private({{.*}}) control(%{{.*}} : i32) = (%{{.*}} : i32) to (%{{.*}} : i32) step (%{{.*}} : i32) { +! CHECK: scf.execute_region no_inline { +! CHECK: acc.loop private({{.*}}) control(%{{.*}} : i32) = (%{{.*}} : i32) to (%{{.*}} : i32) step (%{{.*}} : i32) { +! CHECK: scf.execute_region no_inline { ! CHECK: arith.cmpi eq ! CHECK: cf.cond_br +! CHECK: scf.yield ! CHECK: acc.yield -! CHECK: } ! `acc serial loop collapse(N)` with STOP in body: wrap-in-execute-region hides ! the unstructured if/stop and the three collapsed iterators lower as a single diff --git a/flang/test/Lower/OpenMP/Todo/metadirective-loop-unstructured.f90 b/flang/test/Lower/OpenMP/Todo/metadirective-loop-unstructured.f90 deleted file mode 100644 index 08ebcc7b4c8eff..00000000000000 --- a/flang/test/Lower/OpenMP/Todo/metadirective-loop-unstructured.f90 +++ /dev/null @@ -1,39 +0,0 @@ -! Defer unstructured associated loops until every selection path can give its -! PFT blocks an independent mapping. - -! RUN: split-file %s %t -! RUN: %not_todo_cmd %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=52 -o - %t/static.f90 2>&1 | FileCheck %s -! RUN: %not_todo_cmd %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=52 -o - %t/runtime.f90 2>&1 | FileCheck %s - -! CHECK: not yet implemented: unstructured associated DO in loop-associated METADIRECTIVE variant - -!--- static.f90 -subroutine test_static(n, a, selector) - integer :: n, a(n), selector, i - !$omp metadirective & - !$omp & when(implementation={vendor(llvm)}: do) & - !$omp & otherwise(nothing) - do i = 1, n - go to (10, 20), selector -10 a(i) = 1 - go to 30 -20 a(i) = 2 -30 c... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/225758 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
