[OpenMP] Canonicalization framework #599
Conversation
ichoyjx
requested review from
sscalpone,
jeanPerier,
tskeith,
schweitzpgi,
vjayathirtha-nv,
klausler,
vdonaldson and
psteinfeld
lib/parser/canonicalize-omp.cc
Outdated
| @@ -0,0 +1,127 @@ | |||
| // Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved. | |||
There was a problem hiding this comment.
lib/parser doesn't seem to me to be the right directory for semantic rewriting passes.
There was a problem hiding this comment.
This is really a syntactic operation, so I think this is an appropriate place for it. If it weren't for the probably of labeled DO statements, the parser would be taking care of associating DO loops with OMP directives.
For the same reason, CanonicalizeDo should be in lib/parser. It's already in namespace Fortran::parser, which is confusing for something in lib/semantics. Both of these operations are about constructing a good parse tree and don't depend on any semantic analysis.
There was a problem hiding this comment.
I would be okay with having both of those rewrites in lib/parser, but not just one of them. And they should be driven by the parser, not Semantics::Perform(), if they're not in lib/semantics.
There was a problem hiding this comment.
Yes, that makes sense. I suggest that for this PR, leave canonicalize-omp.cc in lib/parser. When it is merged I'll move canonicalize-do.cc in there too and add a new member function in parser::Parsing that calls both canonicalize functions. I'll call that from the driver and remove the calls from Semantics::Perform().
There was a problem hiding this comment.
If you move canonicalize-do.cc in the parser, this means it will be run even if ValidateLabels would have failed (assuming you keep ValidateLabels in semantics).
I think this is mostly OK since label errors related to labeled do loops will simply prevent the rewriting and errors will be emitted in ValidateLabels afterwards on the remaining labeled do loops. Not 100% sure here, maybe @schweitzpgi can confirm.
The only thing is that we probably want to move the warning related to labeled do loops from ValidateLabels to canonicalize-do.cc because these situations will not get to ValidateLabels anymore.
Swapping ValidateLabels and canonicalize-do.cc should also solve #555
There was a problem hiding this comment.
It may work to commute to work by driving your car in reverse, but that might not be good engineering.
canonicalize-do was written with the assertion that label resolution must have happened. If labels are missing, misplaced, duplicated, etc., etc., the canonicalizer may choke hard, corrupting the parse tree and/or providing little or no feedback to the user. Worse yet, the canonicalizer rewrites the parse tree, necessarily changing its content, which may cause what should be label resolution errors to not be detected by virtue of erroneously changing the input.
This is not the only case where there is a phase ordering in semantics checking and creating/modifying front-end data structures. In general, rewriting the syntax tree blindly and before any sort of correctness testing is performed seems like the wrong approach.
There was a problem hiding this comment.
Sounds like the dependence on label resolution will force DO loop canonicalization to remain in semantics. If this code also depends on successful label resolution checks, it will also need to go into semantics. Otherwise, if we're absolutely sure that it's safe to run without semantics, it can stay in the parser.
There was a problem hiding this comment.
Actually, this code depends on the successful result (structured do-loops) of the DO loop canonicalization.
There was a problem hiding this comment.
Based on the above discussions and after talking with Steve, I've moved this code to lib/semantics for now.
lib/semantics/semantics.cc
Outdated
| @@ -170,6 +171,7 @@ Scope &SemanticsContext::FindScope(parser::CharBlock source) { | |||
| bool Semantics::Perform() { | |||
| return ValidateLabels(context_, program_) && | |||
| parser::CanonicalizeDo(program_) && // force line break | |||
| parser::CanonicalizeOmp(context_.messages(), program_) && | |||
There was a problem hiding this comment.
Does this need to be enable/disabled somehow according to-fopenmp; it seems a waste to make a pass if we know OpenMP is disabled ( I have seen discussion to move this part into the parser, so this comment can probably be left unanswered here and considered when moving this into parser::Parsing by @tskeith).
lib/parser/canonicalize-omp.cc
Outdated
| @@ -0,0 +1,127 @@ | |||
| // Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved. | |||
There was a problem hiding this comment.
If you move canonicalize-do.cc in the parser, this means it will be run even if ValidateLabels would have failed (assuming you keep ValidateLabels in semantics).
I think this is mostly OK since label errors related to labeled do loops will simply prevent the rewriting and errors will be emitted in ValidateLabels afterwards on the remaining labeled do loops. Not 100% sure here, maybe @schweitzpgi can confirm.
The only thing is that we probably want to move the warning related to labeled do loops from ValidateLabels to canonicalize-do.cc because these situations will not get to ValidateLabels anymore.
Swapping ValidateLabels and canonicalize-do.cc should also solve #555
ichoyjx
force-pushed
the
by-mp-parser
branch
2 times, most recently
from
3ab21fd
to
5623d8a
Compare
1. Following Block and Sections constructs, re-structure loop related
constructs into `{Begin, Loop, End}`. Being part of the work in
PR #599, the `Loop` and `End` nodes are optional during parser. They
should be filled in during the phase of `CanonicalizationOfOmp`. This
commit is solely for the parse tree change. So, after this commit,
PR #599 needs to be changed accordingly.
2. Removed parse tree nodes for `END DO` and `END DO SIMD`. Similar to
Block and Sections constructs, `End` node now accepts clauses too,
the validity checks are deferred into Semantics. This is more genernal
and error message could be better.
3. With this commit alone, assertion error would occur when `End` directive
is present, for example `!$OMP END DO` because the `End` node is not
moved into `OpenMPLoopConstruct` yet. Again, PR #599 will handle that.
More tests will be added in PR #599 and during the future Semantics work.
|
Hi all, this PR has been updated with the latest parse tree changes for |
I see several unresolved comments in the conversation above. |
This is mainly designed for loop association work but can be used for others, and `CanonicalizeOmp` must be after `CanonicalizeDo`. At the `Block` level, recognize legal sequence of `OpenMPLoopConstruct`, `DoConstruct`, and `OmpEndLoopDirective`. Move available `DoConstruct` and optional `OmpEndLoopDirective` into `OpenMPLoopConstruct`. Throw error messages if: 1. `DoConstruct` is not following `OpenMPLoopConstruct` 2. `OmpEndLoopDirective` is not following associated do-loop Once this pass this done, Semantics will not proceed if error exists. This can be extended to do the `OmpLoopDirective` and `OpenMPEndLoopDirective` matching checks.
1. extract matching and move part into its own function (once `DoConstruct` is moved, see whether `OpenMPEndLoopDirective` is available) 2. clean up
1. Use a template function to access construct from ExecutionPartConstruct. 2. Simplify the logic
more cleanup
1. Move this code into namespace semantics 2. Some small changes
lib/semantics/canonicalize-omp.h
Outdated
|
|
||
| #include "../parser/message.h" | ||
| #include "semantics.h" |
There was a problem hiding this comment.
Why do you need this include?
…#656) 1. Following Block and Sections constructs, re-structure loop related constructs into `{Begin, Loop, End}`. Being part of the work in PR flang-compiler#599, the `Loop` and `End` nodes are optional during parser. They should be filled in during the phase of `CanonicalizationOfOmp`. This commit is solely for the parse tree change. So, after this commit, PR flang-compiler#599 needs to be changed accordingly. 2. Removed parse tree nodes for `END DO` and `END DO SIMD`. Similar to Block and Sections constructs, `End` node now accepts clauses too, the validity checks are deferred into Semantics. This is more genernal and error message could be better. 3. With this commit alone, assertion error would occur when `End` directive is present, for example `!$OMP END DO` because the `End` node is not moved into `OpenMPLoopConstruct` yet. Again, PR flang-compiler#599 will handle that. More tests will be added in PR flang-compiler#599 and during the future Semantics work.
* [OpenMP] Canonicalization framework This is mainly designed for loop association work but can be used for others, and `CanonicalizeOmp` must be after `CanonicalizeDo`. At the `Block` level, recognize legal sequence of `OpenMPLoopConstruct`, `DoConstruct`, and `OmpEndLoopDirective`. Move available `DoConstruct` and optional `OmpEndLoopDirective` into `OpenMPLoopConstruct`. Throw error messages if: 1. `DoConstruct` is not following `OpenMPLoopConstruct` 2. `OmpEndLoopDirective` is not following associated do-loop Once this pass this done, Semantics will not proceed if error exists. * Update on reviews 1. extract matching and move part into its own function (once `DoConstruct` is moved, see whether `OpenMPEndLoopDirective` is available) 2. Use a template function to access construct from ExecutionPartConstruct. 3. Move this code into namespace semantics
…compiler/f18#656) 1. Following Block and Sections constructs, re-structure loop related constructs into `{Begin, Loop, End}`. Being part of the work in PR flang-compiler/f18#599, the `Loop` and `End` nodes are optional during parser. They should be filled in during the phase of `CanonicalizationOfOmp`. This commit is solely for the parse tree change. So, after this commit, PR flang-compiler/f18#599 needs to be changed accordingly. 2. Removed parse tree nodes for `END DO` and `END DO SIMD`. Similar to Block and Sections constructs, `End` node now accepts clauses too, the validity checks are deferred into Semantics. This is more genernal and error message could be better. 3. With this commit alone, assertion error would occur when `End` directive is present, for example `!$OMP END DO` because the `End` node is not moved into `OpenMPLoopConstruct` yet. Again, PR flang-compiler/f18#599 will handle that. More tests will be added in PR flang-compiler/f18#599 and during the future Semantics work. Original-commit: flang-compiler/f18@8cd1932 Reviewed-on: flang-compiler/f18#656
* [OpenMP] Canonicalization framework This is mainly designed for loop association work but can be used for others, and `CanonicalizeOmp` must be after `CanonicalizeDo`. At the `Block` level, recognize legal sequence of `OpenMPLoopConstruct`, `DoConstruct`, and `OmpEndLoopDirective`. Move available `DoConstruct` and optional `OmpEndLoopDirective` into `OpenMPLoopConstruct`. Throw error messages if: 1. `DoConstruct` is not following `OpenMPLoopConstruct` 2. `OmpEndLoopDirective` is not following associated do-loop Once this pass this done, Semantics will not proceed if error exists. * Update on reviews 1. extract matching and move part into its own function (once `DoConstruct` is moved, see whether `OpenMPEndLoopDirective` is available) 2. Use a template function to access construct from ExecutionPartConstruct. 3. Move this code into namespace semantics Original-commit: flang-compiler/f18@52979f1 Reviewed-on: flang-compiler/f18#599
…compiler/f18#656) 1. Following Block and Sections constructs, re-structure loop related constructs into `{Begin, Loop, End}`. Being part of the work in PR flang-compiler/f18#599, the `Loop` and `End` nodes are optional during parser. They should be filled in during the phase of `CanonicalizationOfOmp`. This commit is solely for the parse tree change. So, after this commit, PR flang-compiler/f18#599 needs to be changed accordingly. 2. Removed parse tree nodes for `END DO` and `END DO SIMD`. Similar to Block and Sections constructs, `End` node now accepts clauses too, the validity checks are deferred into Semantics. This is more genernal and error message could be better. 3. With this commit alone, assertion error would occur when `End` directive is present, for example `!$OMP END DO` because the `End` node is not moved into `OpenMPLoopConstruct` yet. Again, PR flang-compiler/f18#599 will handle that. More tests will be added in PR flang-compiler/f18#599 and during the future Semantics work. Original-commit: flang-compiler/f18@8cd1932 Reviewed-on: flang-compiler/f18#656
* [OpenMP] Canonicalization framework This is mainly designed for loop association work but can be used for others, and `CanonicalizeOmp` must be after `CanonicalizeDo`. At the `Block` level, recognize legal sequence of `OpenMPLoopConstruct`, `DoConstruct`, and `OmpEndLoopDirective`. Move available `DoConstruct` and optional `OmpEndLoopDirective` into `OpenMPLoopConstruct`. Throw error messages if: 1. `DoConstruct` is not following `OpenMPLoopConstruct` 2. `OmpEndLoopDirective` is not following associated do-loop Once this pass this done, Semantics will not proceed if error exists. * Update on reviews 1. extract matching and move part into its own function (once `DoConstruct` is moved, see whether `OpenMPEndLoopDirective` is available) 2. Use a template function to access construct from ExecutionPartConstruct. 3. Move this code into namespace semantics Original-commit: flang-compiler/f18@52979f1 Reviewed-on: flang-compiler/f18#599
This is mainly designed for loop association work but can be used for others,
and
CanonicalizeOmpmust be afterCanonicalizeDo.At the
Blocklevel, recognize legal sequence ofOpenMPLoopConstruct,DoConstruct, andOmpEndLoopDirective. Move availableDoConstructand optional
OmpEndLoopDirectiveintoOpenMPLoopConstruct. Thisrequires the change in
OpenMPLoopConstructdata structure.Throw error messages if:
DoConstructis not followingOpenMPLoopConstructDoConstructdoes not haveLoopControlOmpEndLoopDirectiveis not following associated do-loopOnce this pass this done, Semantics will not proceed if error exists.