Added parser support for Tile Construct .
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| flang/include/flang/Parser/dump-parse-tree.h | ||
|---|---|---|
| 460 ↗ | (On Diff #469229) | |
Comment Actions
- To fix the compilation issue, you will probably have to add the following
diff --git a/flang/lib/Lower/OpenMP.cpp b/flang/lib/Lower/OpenMP.cpp index adada64e3cf4..de74ccd6333b 100644 --- a/flang/lib/Lower/OpenMP.cpp +++ b/flang/lib/Lower/OpenMP.cpp @@ -1711,6 +1711,9 @@ void Fortran::lower::genOpenMPConstruct( &criticalConstruct) { genOMP(converter, eval, criticalConstruct); }, + [&](const auto &) { + TODO(converter.getCurrentLocation(), "New construct"); + }, }, ompConstruct.u); }
- Another way to approach this is to model this as an OpenMPLoopConstruct and not as a separate OpenMPTileConstruct. You can add something like the following,
"DO" >> pure(llvm::omp::Directive::OMPD_do) in https://github.com/llvm/llvm-project/blob/e9754f0211076bab34e5a070cb8eb392a21c0540/flang/lib/Parser/openmp-parsers.cpp#L301
and a case for the tile directive in bool OmpAttributeVisitor::Pre(const parser::OpenMPLoopConstruct &x) to avoid a crash in Semantics.
Did you try this approach and face any issues? Is tile too different from the worksharing loop and other loop like constructs?
Comment Actions
@kiranchandramohan : You mean we should have :
| ProgramStmt -> Name = 'tileloop'
| SpecificationPart
| | ImplicitPart ->
| | DeclarationConstruct -> SpecificationConstruct -> TypeDeclarationStmt
| | | DeclarationTypeSpec -> IntrinsicTypeSpec -> IntegerTypeSpec ->
| | | EntityDecl
| | | | Name = 'i'
| ExecutionPart -> Block
| | ExecutionPartConstruct -> ExecutableConstruct -> OpenMPConstruct -> OpenMPLoopConstruct
| | | OmpBeginLoopDirective
| | | | OmpLoopDirective -> llvm::omp::Directive = tile
| | | | OmpClauseList ->
| | | DoConstruct
| | | | NonLabelDoStmt
| | | | | LoopControl -> LoopBounds
| | | | | | Scalar -> Name = 'i'
| | | | | | Scalar -> Expr = '1_4'
| | | | | | | LiteralConstant -> IntLiteralConstant = '1'
| | | | | | Scalar -> Expr = '9_4'
| | | | | | | LiteralConstant -> IntLiteralConstant = '9'
| | | | Block
| | | | | ExecutionPartConstruct -> ExecutableConstruct -> ActionStmt -> PrintStmt
| | | | | | Format -> Star
| | | | | | OutputItem -> Expr = 'i'
| | | | | | | Designator -> DataRef -> Name = 'i'
| | | | EndDoStmt ->
| | | OmpEndLoopDirective
| | | | OmpLoopDirective -> llvm::omp::Directive = tile
| | | | OmpClauseList ->
| EndProgramStmt ->
For the following code:
program tileloop
integer :: i
!$OMP tile
do i = 1, 9
print*, i
end do
!$OMP END tile
endComment Actions
Yes, that is what I meant. Please think about whether it will be sufficient and whether the tile construct can indeed be modelled as an OpenMPLoopConstruct or whether there are significant differences.
Comment Actions
It looks that the tile construct can be modeled as OpenMPLoopConstruct. But, how it will be linked to OmpLoopDirective? I am getting the following error:
Construct]’
/home/amalik/restored/malik-backup/malik/LLVM/MLIR_FLANG/llvm-project/flang/lib/Parser/openmp-parsers.cpp:510:1: required from here
/home/amalik/restored/malik-backup/malik/LLVM/MLIR_FLANG/llvm-project/flang/include/flang/Parser/parse-tree.h:111:29: error: no matching function for call to ‘std::tuple<Fortran::parser::OmpLoopDirective, Fortran::parser::OmpClauseList>::tuple(std::remove_reference<Fortran::parser::OmpClauseList&>::type)’
111 | classname(Ts &&...args) : t(std::move(args)...) {} \
| ^~~~~~~~~~~~~~~~~~~~~
/home/amalik/restored/malik-backup/malik/LLVM/MLIR_FLANG/llvm-project/flang/include/flang/Parser/parse-tree.h:3818:3: note: in expansion of macro ‘TUPLE_CLASS_BOILERPLATE’
3818 | TUPLE_CLASS_BOILERPLATE(OmpEndTileDirective);by introducing the following in parse-tree.h :
struct OmpBeginTileDirective {
TUPLE_CLASS_BOILERPLATE(OmpBeginTileDirective);
std::tuple<OmpLoopDirective, OmpClauseList> t;
CharBlock source;
};
struct OmpEndTileDirective {
TUPLE_CLASS_BOILERPLATE(OmpEndTileDirective);
std::tuple<OmpLoopDirective, OmpClauseList> t;
CharBlock source;
};Comment Actions
I think you will not need the OmpBeginTileDirective and OmpEndTileDirective if you are following the OmpLoopDirective approach. Why did you add these two structs?
Comment Actions
Looks OK. If you can add the tests suggested by @clementval this will be ready to go in.
| flang/lib/Parser/openmp-parsers.cpp | ||
|---|---|---|
| 280–282 | Nit: Is this an accidental change? | |
| 315 | Nit: This is probably in alphabetical order, can you move this to be in the right order? | |
Comment Actions
I have a question about the test: The following passed the test
! RUN: %flang_fc1 -fdebug-unparse -fopenmp %s | FileCheck --ignore-case %s ! RUN: %flang_fc1 -fdebug-dump-parse-tree -fopenmp %s | FileCheck --check-prefix="PARSE-TREE" %s subroutine openmp_tiles(x, y) integer, intent(inout)::x, y !CHECK: !$omp !$omp tile !CHECK: do do x = 1, 100 call F1(); !CHECK: end do end do; !CHECK: !$omp end !$omp end tile !PARSE-TREE: OpenMPConstruct -> OpenMPLoopConstruct !PARSE-TREE: OmpBeginLoopDirective !PARSE-TREE: OmpLoopDirective -> llvm::omp::Directive = tile END subroutine openmp_tiles
However, when using !CHECK: !$omp tile or !CHECK: !$omp end tile it fails as it does not see the TILE TOKEN.
Comment Actions
This is because there is no entry in unparse.cpp for the tile directive in unparsing OmpLoopDirective. https://github.com/llvm/llvm-project/blob/f387918dd8549331a4f60df70cccd9558eca8df1/flang/lib/Parser/unparse.cpp#L2085
Comment Actions
The build is failing. See inline for issue.
"TILE" >> pure(llvm::omp::Directive::OMPD_tile))
^
/var/lib/buildkite-agent/builds/llvm-project/flang/lib/Parser/type-parser-implementation.h:23:9: note: macro 'TYPE_PARSER' defined here
#define TYPE_PARSER(pexpr) \
^
/var/lib/buildkite-agent/builds/llvm-project/flang/lib/Parser/openmp-parsers.cpp:285:1: error: unknown type name 'TYPE_PARSER'
TYPE_PARSER(sourced(construct<OmpLoopDirective>(first(
^
/var/lib/buildkite-agent/builds/llvm-project/flang/lib/Parser/openmp-parsers.cpp:321:1: error: expected unqualified-id
TYPE_PARSER(sourced(construct<OmpBeginLoopDirective>(
^
/var/lib/buildkite-agent/builds/llvm-project/flang/lib/Parser/type-parser-implementation.h:24:3: note: expanded from macro 'TYPE_PARSER'
template <> \
^Also, please clang-format the following files,
flang/lib/Parser/openmp-parsers.cpp flang/lib/Parser/unparse.cpp
| flang/lib/Parser/openmp-parsers.cpp | ||
|---|---|---|
| 340 | I think you have got the number of brackets wrong in this line or the line above. | |
Comment Actions
@abidmalikwaterloo There is still a clang-format issue causing the CI to fail. Please fix.
Also, can you remove the "(Work in Progress)" wording from the Summary of this patch?
BTW, do you have commit access for submitting this patch?
Comment Actions
There is still an issue with the clang-format of flang/lib/Parser/unparse.cpp that is causing the CI to fail.
I also completely missed that the support for sizes clause is missing. We have to add support for parsing this clause in openmp-parsers.cpp and specify the type in the OMP.td. See relevant changes below. Feel free to use the suggested change if it looks OK.
Please also add a test with the sizes clause.
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp index b4d41c899e96..032e3c6cf8ca 100644 --- a/flang/lib/Parser/openmp-parsers.cpp +++ b/flang/lib/Parser/openmp-parsers.cpp @@ -263,6 +263,8 @@ TYPE_PARSER( "SIMD"_id >> construct<OmpClause>(construct<OmpClause::Simd>()) || "SIMDLEN" >> construct<OmpClause>(construct<OmpClause::Simdlen>( parenthesized(scalarIntConstantExpr))) || + "SIZES" >> construct<OmpClause>(construct<OmpClause::Sizes>( + parenthesized(nonemptyList(scalarIntExpr)))) || "THREADS" >> construct<OmpClause>(construct<OmpClause::Threads>()) || "THREAD_LIMIT" >> construct<OmpClause>(construct<OmpClause::ThreadLimit>( parenthesized(scalarIntExpr))) || diff --git a/llvm/include/llvm/Frontend/OpenMP/OMP.td b/llvm/include/llvm/Frontend/OpenMP/OMP.td index 5f1d335ef04f..cac10c4c690f 100644 --- a/llvm/include/llvm/Frontend/OpenMP/OMP.td +++ b/llvm/include/llvm/Frontend/OpenMP/OMP.td @@ -67,7 +67,11 @@ def OMPC_Private : Clause<"private"> { let clangClass = "OMPPrivateClause"; let flangClass = "OmpObjectList"; } -def OMPC_Sizes: Clause<"sizes"> { let clangClass = "OMPSizesClause"; } +def OMPC_Sizes: Clause<"sizes"> { + let clangClass = "OMPSizesClause"; + let flangClass = "ScalarIntExpr"; + let isValueList = true; +} def OMPC_Full: Clause<"full"> { let clangClass = "OMPFullClause"; } def OMPC_Partial: Clause<"partial"> { let clangClass = "OMPPartialClause"; } def OMPC_FirstPrivate : Clause<"firstprivate"> {
| flang/test/Parser/omp-tile.f90 | ||
|---|---|---|
| 7 | Nit: y is not used. | |
| 13 | Nit: semicolon is not needed. | |
| 15 | Nit: semicolon is not needed. | |
Comment Actions
No, these are not part of any phabricator patch. These are code suggestions for the missing portion to implement support for parsing the tile construct's clauses. You may use it in this patch if it works fine and you are happy with it.
Comment Actions
LGTM. Please fix the clang-format issues in the CI before submitting.
clang-format
changed files:
flang/lib/Parser/openmp-parsers.cpp
flang/lib/Parser/unparse.cpp
Nit: Is this an accidental change?