[Flang][OpenMP] Add Semantics support for Nested OpenMPLoopConstructs #145917
+519
−96
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-semantics @llvm/pr-subscribers-flang-parser Author: Jack Styles (Stylie777) ChangesIn OpenMP Version 5.1, the tile and unroll directives were added. When using these directives, it is possible to nest them within other OpenMP Loop Constructs. This patch enables the semantics to allow for this behaviour on these specific directives. Any nested loops will be stored within the initial Loop Construct until reaching the DoConstruct itself. Relevant tests have been added, and previous behaviour has been retained with no changes. See also, #110008 Patch is 26.86 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/145917.diff 11 Files Affected:
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index 61f97b855b0e5..7aa9250234978 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -5025,7 +5025,7 @@ struct OpenMPLoopConstruct {
TUPLE_CLASS_BOILERPLATE(OpenMPLoopConstruct);
OpenMPLoopConstruct(OmpBeginLoopDirective &&a)
: t({std::move(a), std::nullopt, std::nullopt}) {}
- std::tuple<OmpBeginLoopDirective, std::optional<DoConstruct>,
+ std::tuple<OmpBeginLoopDirective, std::optional<std::variant<DoConstruct, common::Indirection<OpenMPLoopConstruct>>>,
std::optional<OmpEndLoopDirective>>
t;
};
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index 3e865a1ee7185..e81ce5e3dd22f 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -4107,6 +4107,12 @@ static void genOMP(lower::AbstractConverter &converter, lower::SymMap &symTable,
mlir::Location currentLocation =
converter.genLocation(beginLoopDirective.source);
+ auto &optLoopCons = std::get<1>(loopConstruct.t);
+ if(optLoopCons.has_value())
+ if(auto *ompNestedLoopCons{std::get_if<common::Indirection<parser::OpenMPLoopConstruct>>(&*optLoopCons)}) {
+ genOMP(converter, symTable, semaCtx, eval, ompNestedLoopCons->value());
+ }
+
llvm::omp::Directive directive =
std::get<parser::OmpLoopDirective>(beginLoopDirective.t).v;
const parser::CharBlock &source =
diff --git a/flang/lib/Parser/unparse.cpp b/flang/lib/Parser/unparse.cpp
index ed0f227fd5b98..276be878dd44e 100644
--- a/flang/lib/Parser/unparse.cpp
+++ b/flang/lib/Parser/unparse.cpp
@@ -2926,7 +2926,7 @@ class UnparseVisitor {
Walk(std::get<OmpBeginLoopDirective>(x.t));
Put("\n");
EndOpenMP();
- Walk(std::get<std::optional<DoConstruct>>(x.t));
+ Walk(std::get<std::optional<std::variant<DoConstruct, common::Indirection<parser::OpenMPLoopConstruct>>>>(x.t));
Walk(std::get<std::optional<OmpEndLoopDirective>>(x.t));
}
void Unparse(const BasedPointer &x) {
diff --git a/flang/lib/Semantics/canonicalize-omp.cpp b/flang/lib/Semantics/canonicalize-omp.cpp
index 5164f1dc6faab..d6827435173d3 100644
--- a/flang/lib/Semantics/canonicalize-omp.cpp
+++ b/flang/lib/Semantics/canonicalize-omp.cpp
@@ -8,6 +8,7 @@
#include "canonicalize-omp.h"
#include "flang/Parser/parse-tree-visitor.h"
+#include "flang/Parser/parse-tree.h"
// After Loop Canonicalization, rewrite OpenMP parse tree to make OpenMP
// Constructs more structured which provide explicit scopes for later
@@ -106,6 +107,12 @@ class CanonicalizationOfOmp {
return nullptr;
}
+ void missingDoConstruct(parser::OmpLoopDirective &dir) {
+ messages_.Say(dir.source,
+ "A DO loop must follow the %s directive"_err_en_US,
+ parser::ToUpperCaseLetters(dir.source.ToString()));
+ }
+
void RewriteOpenMPLoopConstruct(parser::OpenMPLoopConstruct &x,
parser::Block &block, parser::Block::iterator it) {
// Check the sequence of DoConstruct and OmpEndLoopDirective
@@ -135,31 +142,62 @@ class CanonicalizationOfOmp {
if (auto *doCons{GetConstructIf<parser::DoConstruct>(*nextIt)}) {
if (doCons->GetLoopControl()) {
// move DoConstruct
- std::get<std::optional<parser::DoConstruct>>(x.t) =
+ std::get<std::optional<std::variant<parser::DoConstruct, common::Indirection<parser::OpenMPLoopConstruct>>>>(x.t) =
std::move(*doCons);
nextIt = block.erase(nextIt);
// try to match OmpEndLoopDirective
- if (nextIt != block.end()) {
- if (auto *endDir{
- GetConstructIf<parser::OmpEndLoopDirective>(*nextIt)}) {
- std::get<std::optional<parser::OmpEndLoopDirective>>(x.t) =
- std::move(*endDir);
- block.erase(nextIt);
- }
+ if (auto *endDir{
+ GetConstructIf<parser::OmpEndLoopDirective>(*nextIt)}) {
+ std::get<std::optional<parser::OmpEndLoopDirective>>(x.t) =
+ std::move(*endDir);
+ nextIt = block.erase(nextIt);
}
} else {
messages_.Say(dir.source,
"DO loop after the %s directive must have loop control"_err_en_US,
parser::ToUpperCaseLetters(dir.source.ToString()));
}
+ } else if (auto *ompLoopCons{
+ GetOmpIf<parser::OpenMPLoopConstruct>(*nextIt)}) {
+ // We should allow UNROLL and TILE constructs to be inserted between an OpenMP Loop Construct and the DO loop itself
+ auto &beginDirective =
+ std::get<parser::OmpBeginLoopDirective>(ompLoopCons->t);
+ auto &beginLoopDirective =
+ std::get<parser::OmpLoopDirective>(beginDirective.t);
+ // iterate through the remaining block items to find the end directive for the unroll/tile directive.
+ parser::Block::iterator endIt;
+ endIt = nextIt;
+ while(endIt != block.end()) {
+ if (auto *endDir{
+ GetConstructIf<parser::OmpEndLoopDirective>(*endIt)}) {
+ auto &endLoopDirective = std::get<parser::OmpLoopDirective>(endDir->t);
+ if(endLoopDirective.v == dir.v) {
+ std::get<std::optional<parser::OmpEndLoopDirective>>(x.t) =
+ std::move(*endDir);
+ endIt = block.erase(endIt);
+ continue;
+ }
+ }
+ ++endIt;
+ }
+ if ((beginLoopDirective.v == llvm::omp::Directive::OMPD_unroll ||
+ beginLoopDirective.v == llvm::omp::Directive::OMPD_tile)) {
+ RewriteOpenMPLoopConstruct(*ompLoopCons, block, nextIt);
+ auto &ompLoop = std::get<std::optional<std::variant<parser::DoConstruct, common::Indirection<parser::OpenMPLoopConstruct>>>>(x.t);
+ ompLoop = std::optional<std::variant<parser::DoConstruct, common::Indirection<parser::OpenMPLoopConstruct>>>{
+ std::variant<parser::DoConstruct, common::Indirection<parser::OpenMPLoopConstruct>>{
+ common::Indirection{std::move(*ompLoopCons)}}};
+ nextIt = block.erase(nextIt);
+ }
} else {
- messages_.Say(dir.source,
- "A DO loop must follow the %s directive"_err_en_US,
- parser::ToUpperCaseLetters(dir.source.ToString()));
+ missingDoConstruct(dir);
}
// If we get here, we either found a loop, or issued an error message.
return;
}
+ if (nextIt == block.end()) {
+ missingDoConstruct(dir);
+ }
}
void RewriteOmpAllocations(parser::ExecutionPart &body) {
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 36d4bcb5d99fa..4e71641ae3858 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -761,10 +761,13 @@ void OmpStructureChecker::Enter(const parser::OpenMPLoopConstruct &x) {
}
SetLoopInfo(x);
- if (const auto &doConstruct{
- std::get<std::optional<parser::DoConstruct>>(x.t)}) {
- const auto &doBlock{std::get<parser::Block>(doConstruct->t)};
- CheckNoBranching(doBlock, beginDir.v, beginDir.source);
+ auto &optLoopCons = std::get<1>(x.t);
+ if(optLoopCons.has_value()) {
+ if (const auto &doConstruct{
+ std::get_if<parser::DoConstruct>(&*optLoopCons)}) {
+ const auto &doBlock{std::get<parser::Block>(doConstruct->t)};
+ CheckNoBranching(doBlock, beginDir.v, beginDir.source);
+ }
}
CheckLoopItrVariableIsInt(x);
CheckAssociatedLoopConstraints(x);
@@ -778,6 +781,12 @@ void OmpStructureChecker::Enter(const parser::OpenMPLoopConstruct &x) {
(beginDir.v == llvm::omp::Directive::OMPD_distribute_simd)) {
CheckDistLinear(x);
}
+ if (beginDir.v == llvm::omp::Directive::OMPD_tile) {
+ const auto &clauses{std::get<parser::OmpClauseList>(beginLoopDir.t)};
+ for (auto &clause : clauses.v) {
+
+ }
+ }
}
const parser::Name OmpStructureChecker::GetLoopIndex(
const parser::DoConstruct *x) {
@@ -785,12 +794,15 @@ const parser::Name OmpStructureChecker::GetLoopIndex(
return std::get<Bounds>(x->GetLoopControl()->u).name.thing;
}
void OmpStructureChecker::SetLoopInfo(const parser::OpenMPLoopConstruct &x) {
- if (const auto &loopConstruct{
- std::get<std::optional<parser::DoConstruct>>(x.t)}) {
- const parser::DoConstruct *loop{&*loopConstruct};
- if (loop && loop->IsDoNormal()) {
- const parser::Name &itrVal{GetLoopIndex(loop)};
- SetLoopIv(itrVal.symbol);
+ auto &optLoopCons = std::get<1>(x.t);
+ if (optLoopCons.has_value()) {
+ if (const auto &loopConstruct{
+ std::get_if<parser::DoConstruct>(&*optLoopCons)}) {
+ const parser::DoConstruct *loop{&*loopConstruct};
+ if (loop && loop->IsDoNormal()) {
+ const parser::Name &itrVal{GetLoopIndex(loop)};
+ SetLoopIv(itrVal.symbol);
+ }
}
}
}
@@ -856,8 +868,10 @@ void OmpStructureChecker::CheckIteratorModifier(const parser::OmpIterator &x) {
void OmpStructureChecker::CheckLoopItrVariableIsInt(
const parser::OpenMPLoopConstruct &x) {
- if (const auto &loopConstruct{
- std::get<std::optional<parser::DoConstruct>>(x.t)}) {
+ auto &optLoopCons = std::get<1>(x.t);
+ if (optLoopCons.has_value()) {
+ if (const auto &loopConstruct{
+ std::get_if<parser::DoConstruct>(&*optLoopCons)}) {
for (const parser::DoConstruct *loop{&*loopConstruct}; loop;) {
if (loop->IsDoNormal()) {
@@ -877,6 +891,7 @@ void OmpStructureChecker::CheckLoopItrVariableIsInt(
const auto it{block.begin()};
loop = it != block.end() ? parser::Unwrap<parser::DoConstruct>(*it)
: nullptr;
+ }
}
}
}
@@ -1076,8 +1091,10 @@ void OmpStructureChecker::CheckDistLinear(
// Match the loop index variables with the collected symbols from linear
// clauses.
+ auto &optLoopCons = std::get<1>(x.t);
+ if (optLoopCons.has_value()) {
if (const auto &loopConstruct{
- std::get<std::optional<parser::DoConstruct>>(x.t)}) {
+ std::get_if<parser::DoConstruct>(&*optLoopCons)}) {
for (const parser::DoConstruct *loop{&*loopConstruct}; loop;) {
if (loop->IsDoNormal()) {
const parser::Name &itrVal{GetLoopIndex(loop)};
@@ -1095,6 +1112,7 @@ void OmpStructureChecker::CheckDistLinear(
const auto it{block.begin()};
loop = it != block.end() ? parser::Unwrap<parser::DoConstruct>(*it)
: nullptr;
+ }
}
}
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 885c02e6ec74b..36bea4fbe7ae5 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -1796,10 +1796,13 @@ bool OmpAttributeVisitor::Pre(const parser::OpenMPLoopConstruct &x) {
SetContextAssociatedLoopLevel(GetAssociatedLoopLevelFromClauses(clauseList));
if (beginDir.v == llvm::omp::Directive::OMPD_do) {
- if (const auto &doConstruct{
- std::get<std::optional<parser::DoConstruct>>(x.t)}) {
- if (doConstruct.value().IsDoWhile()) {
- return true;
+ auto &optLoopCons = std::get<1>(x.t);
+ if (optLoopCons.has_value()) {
+ if (const auto &doConstruct{
+ std::get_if<parser::DoConstruct>(&*optLoopCons)}) {
+ if (doConstruct->IsDoWhile()) {
+ return true;
+ }
}
}
}
@@ -1962,48 +1965,64 @@ void OmpAttributeVisitor::PrivatizeAssociatedLoopIndexAndCheckLoopLevel(
bool hasCollapseClause{
clause ? (clause->Id() == llvm::omp::OMPC_collapse) : false};
- const auto &outer{std::get<std::optional<parser::DoConstruct>>(x.t)};
- if (outer.has_value()) {
- for (const parser::DoConstruct *loop{&*outer}; loop && level > 0; --level) {
- if (loop->IsDoConcurrent()) {
- // DO CONCURRENT is explicitly allowed for the LOOP construct so long as
- // there isn't a COLLAPSE clause
- if (isLoopConstruct) {
- if (hasCollapseClause) {
- // hasCollapseClause implies clause != nullptr
- context_.Say(clause->source,
- "DO CONCURRENT loops cannot be used with the COLLAPSE clause."_err_en_US);
+ auto &optLoopCons = std::get<1>(x.t);
+ if (optLoopCons.has_value()) {
+ if (const auto &outer{std::get_if<parser::DoConstruct>(&*optLoopCons)}) {
+ for (const parser::DoConstruct *loop{&*outer}; loop && level > 0; --level) {
+ if (loop->IsDoConcurrent()) {
+ // DO CONCURRENT is explicitly allowed for the LOOP construct so long as
+ // there isn't a COLLAPSE clause
+ if (isLoopConstruct) {
+ if (hasCollapseClause) {
+ // hasCollapseClause implies clause != nullptr
+ context_.Say(clause->source,
+ "DO CONCURRENT loops cannot be used with the COLLAPSE clause."_err_en_US);
+ }
+ } else {
+ auto &stmt =
+ std::get<parser::Statement<parser::NonLabelDoStmt>>(loop->t);
+ context_.Say(stmt.source,
+ "DO CONCURRENT loops cannot form part of a loop nest."_err_en_US);
}
- } else {
- auto &stmt =
- std::get<parser::Statement<parser::NonLabelDoStmt>>(loop->t);
- context_.Say(stmt.source,
- "DO CONCURRENT loops cannot form part of a loop nest."_err_en_US);
- }
- }
- // go through all the nested do-loops and resolve index variables
- const parser::Name *iv{GetLoopIndex(*loop)};
- if (iv) {
- if (auto *symbol{ResolveOmp(*iv, ivDSA, currScope())}) {
- SetSymbolDSA(*symbol, {Symbol::Flag::OmpPreDetermined, ivDSA});
- iv->symbol = symbol; // adjust the symbol within region
- AddToContextObjectWithDSA(*symbol, ivDSA);
}
+ // go through all the nested do-loops and resolve index variables
+ const parser::Name *iv{GetLoopIndex(*loop)};
+ if (iv) {
+ if (auto *symbol{ResolveOmp(*iv, ivDSA, currScope())}) {
+ SetSymbolDSA(*symbol, {Symbol::Flag::OmpPreDetermined, ivDSA});
+ iv->symbol = symbol; // adjust the symbol within region
+ AddToContextObjectWithDSA(*symbol, ivDSA);
+ }
- const auto &block{std::get<parser::Block>(loop->t)};
- const auto it{block.begin()};
- loop = it != block.end() ? GetDoConstructIf(*it) : nullptr;
+ const auto &block{std::get<parser::Block>(loop->t)};
+ const auto it{block.begin()};
+ loop = it != block.end() ? GetDoConstructIf(*it) : nullptr;
+ }
+ }
+ CheckAssocLoopLevel(level, GetAssociatedClause());
+ } else if (const auto &loop{std::get_if<common::Indirection<parser::OpenMPLoopConstruct>>(&*optLoopCons)}) {
+ auto &beginDirective =
+ std::get<parser::OmpBeginLoopDirective>(loop->value().t);
+ auto &beginLoopDirective =
+ std::get<parser::OmpLoopDirective>(beginDirective.t);
+ if ((beginLoopDirective.v != llvm::omp::Directive::OMPD_unroll &&
+ beginLoopDirective.v != llvm::omp::Directive::OMPD_tile)) {
+ context_.Say(GetContext().directiveSource,
+ "Only UNROLL or TILE constructs are allowed between an OpenMP Loop Construct and a DO construct"_err_en_US,
+ parser::ToUpperCaseLetters(llvm::omp::getOpenMPDirectiveName(GetContext().directive, version).str()));
+ } else {
+ PrivatizeAssociatedLoopIndexAndCheckLoopLevel(loop->value());
}
+ } else {
+ context_.Say(GetContext().directiveSource,
+ "A DO loop must follow the %s directive"_err_en_US,
+ parser::ToUpperCaseLetters(
+ llvm::omp::getOpenMPDirectiveName(GetContext().directive, version)
+ .str()));
}
- CheckAssocLoopLevel(level, GetAssociatedClause());
- } else {
- context_.Say(GetContext().directiveSource,
- "A DO loop must follow the %s directive"_err_en_US,
- parser::ToUpperCaseLetters(
- llvm::omp::getOpenMPDirectiveName(GetContext().directive, version)
- .str()));
}
}
+
void OmpAttributeVisitor::CheckAssocLoopLevel(
std::int64_t level, const parser::OmpClause *clause) {
if (clause && level != 0) {
diff --git a/flang/test/Lower/OpenMP/nested-loop-transformation-construct01.f90 b/flang/test/Lower/OpenMP/nested-loop-transformation-construct01.f90
new file mode 100644
index 0000000000000..b24e2d62ac2aa
--- /dev/null
+++ b/flang/test/Lower/OpenMP/nested-loop-transformation-construct01.f90
@@ -0,0 +1,20 @@
+! Test to ensure TODO message is emitted for tile OpenMP 5.1 Directives when they are nested.
+
+!RUN: not %flang -fopenmp -fopenmp-version=51 %s 2<&1 | FileCheck %s
+
+subroutine loop_transformation_construct
+ implicit none
+ integer :: I = 10
+ integer :: x
+ integer :: y(I)
+
+ !$omp do
+ !$omp tile
+ do i = 1, I
+ y(i) = y(i) * 5
+ end do
+ !$omp end tile
+ !$omp end do
+end subroutine
+
+!CHECK: not yet implemented: Unhandled loop directive (tile)
diff --git a/flang/test/Lower/OpenMP/nested-loop-transformation-construct02.f90 b/flang/test/Lower/OpenMP/nested-loop-transformation-construct02.f90
new file mode 100644
index 0000000000000..bbb16a6a699e9
--- /dev/null
+++ b/flang/test/Lower/OpenMP/nested-loop-transformation-construct02.f90
@@ -0,0 +1,20 @@
+! Test to ensure TODO message is emitted for unroll OpenMP 5.1 Directives when they are nested.
+
+!RUN: not %flang -fopenmp -fopenmp-version=51 %s 2<&1 | FileCheck %s
+
+program loop_transformation_construct
+ implicit none
+ integer, parameter :: I = 10
+ integer :: x
+ integer :: y(I)
+
+ !$omp do
+ !$omp unroll
+ do x = 1, I
+ y(x) = y(x) * 5
+ end do
+ !$omp end unroll
+ !$omp end do
+end program loop_transformation_construct
+
+!CHECK: not yet implemented: Unhandled loop directive (unroll)
diff --git a/flang/test/Parser/OpenMP/loop-transformation-construct01.f90 b/flang/test/Parser/OpenMP/loop-transformation-construct01.f90
new file mode 100644
index 0000000000000..baffc2f6e2f1e
--- /dev/null
+++ b/flang/test/Parser/OpenMP/loop-transformation-construct01.f90
@@ -0,0 +1,74 @@
+! Test the Parse Tree to ensure the OpenMP Loop Transformation Constructs nest correctly with 1 nested loop.
+
+! RUN: %flang_fc1 -fdebug-dump-parse-tree -fopenmp -fopenmp-version=51 %s | FileCheck %s --check-prefix=CHECK-PARSE
+! RUN: %flang_fc1 -fdebug-unparse -fopenmp -fopenmp-version=51 %s | FileCheck %s --check-prefix=CHECK-UNPARSE
+
+subroutine loop_transformation_construct
+ implicit none
+ integer :: I = 10
+ integer :: x
+ integer :: y(I)
+
+ !$omp do
+ !$omp unroll
+ do i = 1, I
+ y(i) = y(i) * 5
+ end do
+ !$omp end unroll
+ !$omp end do
+end subroutine
+
+!CHECK-PARSE: | ExecutionPart -> Block
+!CHECK-PARSE-NEXT: | | ExecutionPartConstruct -> ExecutableConstruct -> OpenMPConstruct -> OpenMPLoopConstruct
+!CHECK-PARSE-NEXT: | | | OmpBeginLoopDirective
+!CHECK-PARSE-NEXT: | | | | OmpLoopDirective -> llvm::omp::Directive = do
+!CHECK-PARSE-NEXT: | | | | OmpClauseList ->
+!CHECK-PARSE-NEXT: | | | OpenMPLoopConstruct
+!CHECK-PARSE-NEXT: | | | | OmpBeginLoopDirective
+!CHECK-PARSE-NEXT: | | | | | OmpLoopDirective -> llvm::omp::Directive = unroll
+!CHECK-PARSE-NEXT: | | | | | OmpClauseList ->
+!CHECK-PARSE-NEXT: | | | | DoConstruct
+!CHECK-PARSE-NEXT: | | | | | NonLabelDoStmt
+!CHECK-PARSE-NEXT: | | | | | | LoopControl -> LoopBounds
+!CHECK-PARSE-NEXT: | | | | | | | Scalar -> Name = 'i'
+!CHECK-PARSE-NEXT: | | | | | | | Scalar -> Expr = '1_4'
+!CHECK-PARSE-NEXT: | | | | | | | | LiteralConstant -> IntLiteralConstant = '1'
+!CHECK-PARSE-NEXT: | | | | | | | Scalar -> Expr = 'i'
+!CHECK-PARSE-NEXT: | | | | | | | | Designator -> DataRef -> Name = 'i'
+!CHECK-PARSE-NEXT: | | | | | Block
+!CHECK-PARSE-NEXT: | | | | | | ExecutionPartConstruct -> ExecutableConstruct -> ActionStmt -> AssignmentStmt = 'y(int(i,kind=8))=5_4*y(int(i,kind=8))'
+!CHECK-PARSE-NEXT: | | | | | | | Variable = 'y(int(i,kind=8))'
+!CHECK-PARSE-NEXT: | | | | | | | | Designator -> DataRef -> ArrayElement
+!CHECK-PARSE-NEXT: | | | | | | | | | DataRef -> Name = 'y'
+!CHECK-PARSE-NEXT: | | | | | | | | | SectionSubscript -> Integer -> Expr = 'i'
+!CHECK-PARSE-NEXT: |...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
tblah
reviewed
Jun 26, 2025
|
I have a question about nested loop constructs and checking the legality of combining them. For example doing full unrolling and then tiling would be illegal since if a loop is unrolled there is nothing to tile. Another example that I ran into is collapse and tiling, since collapse assumes independent loops, but only the outer loops after tiling are independent. At some point in the front-end there has to be a checker that makes sure that the nested loop constructs are legal and can report meaningful error messages if they are not. Do you have any ideas how this can be done? Maybe a separate pass would be required that somehow tracks certain properties about the resulting loops that are created from a transform? |
kparzysz
reviewed
Jun 26, 2025
Comment on lines
169
to
176
| if ((beginLoopDirective.v == llvm::omp::Directive::OMPD_unroll || | ||
| beginLoopDirective.v == llvm::omp::Directive::OMPD_tile)) { |
flang/lib/Lower/OpenMP/OpenMP.cpp
Outdated
| @@ -4107,6 +4107,14 @@ static void genOMP(lower::AbstractConverter &converter, lower::SymMap &symTable, | |||
| mlir::Location currentLocation = | |||
| converter.genLocation(beginLoopDirective.source); | |||
|
|
|||
| auto &optLoopCons = std::get<1>(loopConstruct.t); | |||
There was a problem hiding this comment.
Please spell out the type in <>.
Comment on lines
5029
to
5030
| std::optional< | ||
| std::variant<DoConstruct, common::Indirection<OpenMPLoopConstruct>>>, |
There was a problem hiding this comment.
You can add a type alias for this, like
using NestedConstruct = std::variant<...>;
std::tuple<OmpBeginLoopDirective, std::optional<NestedConstruct>, ...
The full thing is spelled out in several places, it would make the code more readable.
| std::get<std::optional<parser::DoConstruct>>(x.t)}) { | ||
| const auto &doBlock{std::get<parser::Block>(doConstruct->t)}; | ||
| CheckNoBranching(doBlock, beginDir.v, beginDir.source); | ||
| auto &optLoopCons = std::get<1>(x.t); |
There was a problem hiding this comment.
Same here, please put the type name in <>. It's longer, but it makes it a bit easier to read, especially since you have auto in the declaration.
| if (loop && loop->IsDoNormal()) { | ||
| const parser::Name &itrVal{GetLoopIndex(loop)}; | ||
| SetLoopIv(itrVal.symbol); | ||
| auto &optLoopCons = std::get<1>(x.t); |
| @@ -856,27 +862,30 @@ void OmpStructureChecker::CheckIteratorModifier(const parser::OmpIterator &x) { | |||
|
|
|||
| void OmpStructureChecker::CheckLoopItrVariableIsInt( | |||
| const parser::OpenMPLoopConstruct &x) { | |||
| if (const auto &loopConstruct{ | |||
| std::get<std::optional<parser::DoConstruct>>(x.t)}) { | |||
| auto &optLoopCons = std::get<1>(x.t); | |||
In OpenMP Version 5.1, the tile and unroll directives were added. When using these directives, it is possible to nest them within other OpenMP Loop Constructs. This patch enables the semantics to allow for this behaviour on these specific directives. Any nested loops will be stored within the initial Loop Construct until reaching the DoConstruct itself. Relevant tests have been added, and previous behaviour has been retained with no changes. See also, llvm#110008
Stylie777
force-pushed
the
loop_transformation_constructs
branch
from
e075bfb to
fec9503
Compare
Stylie777
commented
Jun 27, 2025
Comment on lines
5029
to
5030
| std::optional< | ||
| std::variant<DoConstruct, common::Indirection<OpenMPLoopConstruct>>>, |
flang/lib/Lower/OpenMP/OpenMP.cpp
Outdated
| @@ -4107,6 +4107,14 @@ static void genOMP(lower::AbstractConverter &converter, lower::SymMap &symTable, | |||
| mlir::Location currentLocation = | |||
| converter.genLocation(beginLoopDirective.source); | |||
|
|
|||
| auto &optLoopCons = std::get<1>(loopConstruct.t); | |||
Comment on lines
169
to
176
| if ((beginLoopDirective.v == llvm::omp::Directive::OMPD_unroll || | ||
| beginLoopDirective.v == llvm::omp::Directive::OMPD_tile)) { |
| std::get<std::optional<parser::DoConstruct>>(x.t)}) { | ||
| const auto &doBlock{std::get<parser::Block>(doConstruct->t)}; | ||
| CheckNoBranching(doBlock, beginDir.v, beginDir.source); | ||
| auto &optLoopCons = std::get<1>(x.t); |
| if (loop && loop->IsDoNormal()) { | ||
| const parser::Name &itrVal{GetLoopIndex(loop)}; | ||
| SetLoopIv(itrVal.symbol); | ||
| auto &optLoopCons = std::get<1>(x.t); |
| @@ -856,27 +862,30 @@ void OmpStructureChecker::CheckIteratorModifier(const parser::OmpIterator &x) { | |||
|
|
|||
| void OmpStructureChecker::CheckLoopItrVariableIsInt( | |||
| const parser::OpenMPLoopConstruct &x) { | |||
| if (const auto &loopConstruct{ | |||
| std::get<std::optional<parser::DoConstruct>>(x.t)}) { | |||
| auto &optLoopCons = std::get<1>(x.t); | |||
|
@jsjodin Regarding your comment. This is not something I had considered. I could look at adding a check here? If we have an With the second example you have given, I am not sure if this can be dealt with here and that might require a new pass to check this. I will take a look and see whats possible in the scope I suspect the first point is possible, the second may need some level of processing in the lowering. Admittedly my experience with Flang is limited, having only begun working on this part of the LLVM Project 4 weeks ago, so if others have other ideas please bring them forward. |
|
I think having the checking done in a separate pass makes sense and the same module could also provide utility functions that could be used for codegen. The problem I had to handle was the combination of collapse and tile and calculate how many loops that need considered to be combined into a single loop nest op. That happened in a couple of places in the code. Another issue is restrictions on what these transforms can do in the OpenMPIRBuilder. What is legal or not may vary over time, so having a central place where this is encoded would be useful. Another question is if the checking should implemented in LLVM similar to how directive and clause handling is done? The same problem has to be solved for Clang and maybe MLIR depending on other possible front-ends. Edit: I think for now if there is a simple check that can be added in your PR you might want to do that, but if it is complicated it should probably be done in a separate PR. |
|
@jsjodin I think for other cases where it is more involved, a new pass/PR is more sensible. I don't think solving this in the backend is a bad idea, because then as you pointed out it would deal with MLIR/Clang (and any other frontend that uses llvm). I suspect the new pass could be implemented there and it may remove the need for some of the checks we are doing currently in the frontend of clang. Either way, it's a subject for another PR. |
During review, it was noted that tiling after unrolling should not be possible, so a semantics check is now implemented for this. This also refactors the error messages to be lambda functions where an error message can be called from multiple places.
|
I don't know of a way to generate proper diagnostic messages after semantics. Obviously we can print to stderr and stop the compiler but we loose all of the usual formatting and printing of source code in standard error messages. I think this needs to be done in flang semantics if at all possible (possibly using utilities shared with other places). |
tblah
reviewed
Jun 27, 2025
kparzysz
reviewed
Jun 27, 2025
Comment on lines
80
to
82
| !$omp unroll | ||
| !ERROR: If a loop construct has been fully unrolled, it cannot then be tiled | ||
| !$omp tile |
I think that there may need to be some kind of data structure defined in LLVM that can be constructed and passed to the checker from the front-end, possibly with some callbacks, and then the front-end can report the error. I haven't thought a lot about how to do it for the loop transforms, but it is essentially how it is done with some of the offloading support in the OpenMPIRBuilder. |
|
The canonicalization passes run early on in the semantic checks. The check-omp-structure executes later on. As long as the canonicalization doesn't lose information (source locations in particular), it doesn't need to concern itself with checking the legality of combining constructs. This can be done in check-omp-structure. |
Review comments noted that I had not got the order that unroll then tiling is defined in OpenMP so this has been corrected. The failing windows test has also been addressed. Finally, variable names have been updated so it is easier to follow
kparzysz
reviewed
Jun 30, 2025
| @@ -125,6 +126,16 @@ class CanonicalizationOfOmp { | |||
| parser::Block::iterator nextIt; | |||
| auto &beginDir{std::get<parser::OmpBeginLoopDirective>(x.t)}; | |||
| auto &dir{std::get<parser::OmpLoopDirective>(beginDir.t)}; | |||
| auto missingDoConstruct = [](auto &dir, auto &messages_) { | |||
There was a problem hiding this comment.
Parameters shouldn't have a trailing underscore.
| "A DO loop must follow the %s directive"_err_en_US, | ||
| parser::ToUpperCaseLetters(dir.source.ToString())); | ||
| }; | ||
| auto tileUnrollError = [](auto &dir, auto &messages_) { |
| @@ -4231,6 +4231,15 @@ static void genOMP(lower::AbstractConverter &converter, lower::SymMap &symTable, | |||
| mlir::Location currentLocation = | |||
| converter.genLocation(beginLoopDirective.source); | |||
|
|
|||
| auto &optLoopCons = | |||
| std::get<std::optional<parser::NestedConstruct>>(loopConstruct.t); | |||
| if (optLoopCons.has_value()) | |||
There was a problem hiding this comment.
Please add braces here. This is the common convention even for single statements when the statement contains its own sub-statements.
Comment on lines
+2011
to
+2012
| if ((beginLoopDirective.v != llvm::omp::Directive::OMPD_unroll && | ||
| beginLoopDirective.v != llvm::omp::Directive::OMPD_tile)) { |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In OpenMP Version 5.1, the tile and unroll directives were added. When using these directives, it is possible to nest them within other OpenMP Loop Constructs. This patch enables the semantics to allow for this behaviour on these specific directives. Any nested loops will be stored within the initial Loop Construct until reaching the DoConstruct itself.
Relevant tests have been added, and previous behaviour has been retained with no changes.
See also, #110008