Changes to add an executable construct stack #686

psteinfeld · 2019-08-26T23:35:32Z

I added a stack of ExecutableConstruct's to SemanticsContext along with
functions to push and pop constructs. I added code to the SemanticsVisitor
to use these new functions. I also added functions Pre and Post functions
for UnlabeledStatement's so that we could isolate the source positions for
statements embedded in "if" statements to improve error messages.

I also added code to check-do.[h,cc] to use this new infrastructure to check
for CYCLE and EXIT statements that are not contained within DO constructs
along with a test.

ichoyjx · 2019-08-26T23:51:08Z

Could you do a quick test of the following code:

  !$omp parallel do
  do i = 1, 10
     if (i == 4) cycle
  end do
end

vjayathirtha-nv · 2019-08-27T00:40:47Z

lib/semantics/semantics.h

@@ -120,6 +121,12 @@ class SemanticsContext {
  const Scope &FindScope(parser::CharBlock) const;
  Scope &FindScope(parser::CharBlock);

+  const std::vector<const parser::ExecutableConstruct *> executables() const {


This is would create a copy of executables_ if not for copy elision.
Question for other reviewers: As a general rule, do we depend on such optimizations or code for non existence of such optimizations ?

Unless there is a reason to return a copy I think it should return a const reference.

Yes, you can depend on RVO in C++17. That said, this routine either needs to add a & or remove a const.

Thanks. I'll make this a reference.

vjayathirtha-nv · 2019-08-27T00:41:04Z

lib/semantics/check-do.cc

@@ -647,4 +647,21 @@ void DoChecker::Leave(const parser::DoConstruct &x) {
  doContext.Check(x);
 }

+void DoChecker::CheckLoneStmt(const char *stmtString) const {


CheckLoStmt doesn't really say what it's doing. How about CheckInsideDO() or something like that?

Good suggestion. I'll make that change.

Note that, in my latest version of the code, I added the function InsideDoConstruct() to the classSemanticsContext which supersedes this code.

lib/semantics/semantics.cc

tskeith · 2019-08-27T01:23:07Z

lib/semantics/semantics.h

@@ -20,6 +20,7 @@
 #include "../evaluate/intrinsics.h"
 #include "../parser/features.h"
 #include "../parser/message.h"
+#include "../parser/parse-tree.h"


You only need a forward declaration of ExecutableConstruct, not all of parse-tree.h.

Thanks. I'll change that.

lib/semantics/check-do.cc

psteinfeld · 2019-08-27T16:29:58Z

Could you do a quick test of the following code:
  !$omp parallel do
  do i = 1, 10
     if (i == 4) cycle
  end do
end

Brian, I'm not sure how to construct a complete test. Here's the complete source code for a test file that I constructed:

subroutine s1()
!$omp parallel do
do i = 1, 10
if (i == 4) cycle
end do
!$omp end
end subroutine s1

It compiles without error.

klausler · 2019-08-27T17:21:58Z

lib/semantics/semantics.cc

+}
+
+void SemanticsContext::PopExecutable() {
+  CHECK(executables_.size() > 0);


!empty() is what you're trying to say, I think.

Thanks. I'll change this.

klausler · 2019-08-27T17:23:41Z

lib/semantics/check-do.cc

@@ -647,4 +647,21 @@ void DoChecker::Leave(const parser::DoConstruct &x) {
  doContext.Check(x);
 }

+void DoChecker::CheckLoneStmt(const char *stmtString) const {
+  for (auto &executable : context_.executables()) {


Use const auto & and make sure that executables() returns a const &.

Thanks. I'll make those changes.

klausler · 2019-08-27T17:25:29Z

lib/semantics/semantics.h

@@ -136,6 +143,7 @@ class SemanticsContext {
  evaluate::FoldingContext foldingContext_{defaultKinds_};

  bool CheckError(bool);
+  std::vector<const parser::ExecutableConstruct *> executables_;


This vector represents a stack, but neither its name nor its type mention "stack".

Good suggestion. I'll change the name of the data member and accessor to executableStack.

How about the type?

If the use case is only to lookup top of the stack then std::stack should suffice. However, if one needs to peel the layers for more information then (either now or in the future), implementing a stack using std::vector would be better, just like it is right now with ompContext_

I'm pretty sure that we'll need the more general capabilities of std::vector as we add more semantic checks.

tskeith · 2019-08-27T17:57:49Z

lib/semantics/check-do.h


 private:
+  void CheckLoneStmt(const char *const) const;


The second const here doesn't do anything.

Thanks for spotting this. I'll fix it.

Note that this function has been eliminated in the latest version of the code.

klausler · 2019-08-29T23:32:05Z

lib/semantics/semantics.cc

@@ -168,6 +298,24 @@ Scope &SemanticsContext::FindScope(parser::CharBlock source) {
  }
 }

+void SemanticsContext::PushConstruct(const ConstructNode &construct) {


I'm actually surprised that C++ implicitly converts the pointers at the various call sites into a ConstructNode that can be passed by reference.

This function might be more clear if it used move semantics.

I want to make sure that I understand your suggestion. Are you saying that, for example, the code for the Pre function for a DoConstruct should look like this?

bool Pre(const parser::DoConstruct &doConstruct) { ConstructNode doNode{std::move(&doConstruct)}; context_.PushConstruct(doNode); Enter(doConstruct); return true; }

No, that doesn't work. I meant something like context_.PushConstruct(ConstructNode{doConstruct}); in Pre() with the definition of PushConstruct accepting a ConstructNode &&construct argument.

Thanks, Peter. I'll change the Pre() functions.

Take a look at the version in PR #708

The template suggestion doesn't have anything to do with PushConstruct or constructStack_ being templated. This PR has roughly 10 Pre() and Post() routines manually defined, functionally they all do exactly the same thing. Instead you could use a templated routine each for Pre() and Post() .

Please disregard my earlier comment. I misunderstood things.

Thanks, Tim. I'll change my code to match Brian's.

klausler · 2019-08-29T23:32:50Z

lib/semantics/semantics.cc

+}
+
+void SemanticsContext::PopConstruct() {
+  CHECK(!constructStack_.empty());


This CHECK may not add any value; the pop_back() will crash if the stack is empty.

CHECK has the advantage that the message coming from the compiler contains that name of the file and source line of the failing check. When pop_back() fails, the compiler just produces a segmentation fault with no indication of the location of the failure.

klausler · 2019-08-29T23:34:13Z

lib/semantics/semantics.cc

+}
+
+bool SemanticsContext::InsideDoConstruct() const {
+  for (const ConstructNode construct : constructStack_) {


Rather than looping over the entries in the stack and looking for DO loops, you could maintain a count of active DO loops.

Actually, that was my original implementation. When I reviewed that with Tim, he suggested using a stack. The stack is heavier weight, but it has the promise of being useful in other kinds of semantic analysis.

You can still maintain a counter.

Or maybe a cumulative ancestor set for all ancestor types, not just do loops. That is, for top level constructs, this set is empty. For nested constructs, the set is the parent's set plus the parent's type. That way, queries of the form "this construct has at least one kind X ancestor" can be answered without searching. And queries of the form "this construct is immediately nested in a kind X parent' can also be directly answered by looking one up the stack. Those two queries probably cover a large majority of all potential queries.

Val's suggestion is more efficient compared to the stack approach. However one would still have to traverse up to get a count of active DO loops. So a cumulative ancestor set + depth record for DO (and any other construct that needs its depth saved) seems like the best idea.

Thanks for the suggestions. One of the checks I plan to implement (although not in this pull request) is to semantic checking for CYCLE do-construct-name. For that case, I'll need to walk up the currently active set of nested DO constructs looking for one with do-construct-name. For just checking a lone CYCLE statement, a counter is sufficient and more efficient. Since I don't think I can get rid of the stack, and since I believe that it's sufficient for the cases of CYCLE statements with and without do-construct-names, I plan to just go with a stack. I'll keep the notion of counters in mind as I add the rest of the checks, though.

psteinfeld · 2019-09-10T18:55:13Z

I changed the implementation of PushConstruct() to match Brian's -- to be
a template that accepts any kind of node and do all of the work of creating the
ConstructNode and putting it on the stack.

klausler · 2019-09-10T19:24:54Z

lib/semantics/semantics.cc

+    context_.set_location(std::nullopt);
+  }
+
+  bool Pre(const parser::AssociateConstruct &associateConstruct) {


It would be great if you could combine all of these repetitive Pre overloads. It's hard to see the differences between them, if any, and it's hard to tell which ones are paired up with Post overloads and which are not.

Specifically, you could augment the default Pre() and Post() member function templates above (ca. line #60) with something like

if constexpr (common::HasMember<const N *, ConstructNode>) { context_.PushConstruct(node); }

Peter, you're a genius! I wanted to combine those calls, but I couldn't figure out how to do it.

I added a stack of ExecutableConstruct's to SemanticsContext along with functions to push and pop constructs. I added code to the SemanticsVisitor to use these new functions. I also added functions Pre and Post functions for UnlabeledStatement's so that we could isolate the source positions for statements embedded in "if" statements to improve error messages. I also added code to check-do.[h,cc] to use this new infrastructure to check for CYCLE and EXIT statements that are not contained within DO constructs along with a test.

The most significant change is that I replaced the stack of ExecutableConstruct's with a stack composed of ConstructNode's, each of which is a variant of the constructs that made up the type ExecutableConstruct. This change allows the nodes of the stack to be extended to include the types needed for OMP semantic checking. I also extended the existing test to include some correct DO loops with CYCLE and EXIT statements to test the code more completely.

I changed the interface of ```PushConstruct()``` to take an rvalue reference as its only parameter and made the construction of the ```ConstructNode```s explicit in all of the ```Pre()``` functions for the various construct types.

creation of the ```ConstructNode``` into ```PushConstruct()```.

…s that call `PushConstruct()``` and ```PopConstruct()``` following a genius suggestion from Peter.

psteinfeld · 2019-09-10T22:04:22Z

I combined all of the functions that called PushConstruct() and PopConstruct() based on a suggestion from Peter.

I added a stack of ExecutableConstruct's to SemanticsContext along with functions to push and pop constructs. I added code to the SemanticsVisitor to use these new functions. I also added functions Pre and Post functions for UnlabeledStatement's so that we could isolate the source positions for statements embedded in "if" statements to improve error messages. I also added code to check-do.[h,cc] to use this new infrastructure to check for CYCLE and EXIT statements that are not contained within DO constructs along with a test. Original-commit: flang-compiler/f18@b8370bd Reviewed-on: flang-compiler/f18#686 Tree-same-pre-rewrite: false

The most significant change is that I replaced the stack of ExecutableConstruct's with a stack composed of ConstructNode's, each of which is a variant of the constructs that made up the type ExecutableConstruct. This change allows the nodes of the stack to be extended to include the types needed for OMP semantic checking. I also extended the existing test to include some correct DO loops with CYCLE and EXIT statements to test the code more completely. Original-commit: flang-compiler/f18@d26f34e Reviewed-on: flang-compiler/f18#686 Tree-same-pre-rewrite: false

I changed the interface of ```PushConstruct()``` to take an rvalue reference as its only parameter and made the construction of the ```ConstructNode```s explicit in all of the ```Pre()``` functions for the various construct types. Original-commit: flang-compiler/f18@f8be813 Reviewed-on: flang-compiler/f18#686 Tree-same-pre-rewrite: false

…he implicit creation of the ```ConstructNode``` into ```PushConstruct()```. Original-commit: flang-compiler/f18@3984566 Reviewed-on: flang-compiler/f18#686 Tree-same-pre-rewrite: false

…functions that call `PushConstruct()``` and ```PopConstruct()``` following a genius suggestion from Peter. Original-commit: flang-compiler/f18@be2a03e Reviewed-on: flang-compiler/f18#686

…/ps-stmt-stack Changes to add an executable construct stack Original-commit: flang-compiler/f18@2fc28c7 Reviewed-on: flang-compiler/f18#686

I added a stack of ExecutableConstruct's to SemanticsContext along with functions to push and pop constructs. I added code to the SemanticsVisitor to use these new functions. I also added functions Pre and Post functions for UnlabeledStatement's so that we could isolate the source positions for statements embedded in "if" statements to improve error messages. I also added code to check-do.[h,cc] to use this new infrastructure to check for CYCLE and EXIT statements that are not contained within DO constructs along with a test. Original-commit: flang-compiler/f18@b8370bd Reviewed-on: flang-compiler/f18#686 Tree-same-pre-rewrite: false

The most significant change is that I replaced the stack of ExecutableConstruct's with a stack composed of ConstructNode's, each of which is a variant of the constructs that made up the type ExecutableConstruct. This change allows the nodes of the stack to be extended to include the types needed for OMP semantic checking. I also extended the existing test to include some correct DO loops with CYCLE and EXIT statements to test the code more completely. Original-commit: flang-compiler/f18@d26f34e Reviewed-on: flang-compiler/f18#686 Tree-same-pre-rewrite: false

I changed the interface of ```PushConstruct()``` to take an rvalue reference as its only parameter and made the construction of the ```ConstructNode```s explicit in all of the ```Pre()``` functions for the various construct types. Original-commit: flang-compiler/f18@f8be813 Reviewed-on: flang-compiler/f18#686 Tree-same-pre-rewrite: false

…he implicit creation of the ```ConstructNode``` into ```PushConstruct()```. Original-commit: flang-compiler/f18@3984566 Reviewed-on: flang-compiler/f18#686 Tree-same-pre-rewrite: false

…functions that call `PushConstruct()``` and ```PopConstruct()``` following a genius suggestion from Peter. Original-commit: flang-compiler/f18@be2a03e Reviewed-on: flang-compiler/f18#686

psteinfeld requested review from ichoyjx, jeanPerier, tskeith, schweitzpgi, vjayathirtha-nv, klausler and vdonaldson August 26, 2019 23:35

vjayathirtha-nv reviewed Aug 27, 2019

View reviewed changes

tskeith reviewed Aug 27, 2019

View reviewed changes

lib/semantics/semantics.cc Show resolved Hide resolved

tskeith reviewed Aug 27, 2019

View reviewed changes

lib/semantics/check-do.cc Show resolved Hide resolved

klausler reviewed Aug 27, 2019

View reviewed changes

tskeith reviewed Aug 27, 2019

View reviewed changes

psteinfeld force-pushed the ps-stmt-stack branch from 1a06fe2 to d699c47 Compare August 29, 2019 23:11

psteinfeld requested review from tskeith, klausler and vjayathirtha-nv August 29, 2019 23:15

klausler reviewed Aug 29, 2019

View reviewed changes

psteinfeld force-pushed the ps-stmt-stack branch from d699c47 to 30ab8e6 Compare September 10, 2019 18:45

psteinfeld requested a review from klausler September 10, 2019 18:55

klausler reviewed Sep 10, 2019

View reviewed changes

psteinfeld added 5 commits September 10, 2019 14:45

Responses to review comments

f8be813

I changed the interface of ```PushConstruct()``` to take an rvalue reference as its only parameter and made the construction of the ```ConstructNode```s explicit in all of the ```Pre()``` functions for the various construct types.

Changed PushConstruct() to be a template, which moves the implicit

3984566

creation of the ```ConstructNode``` into ```PushConstruct()```.

Combined the implementations to Pre() and Post() function…

be2a03e

…s that call `PushConstruct()``` and ```PopConstruct()``` following a genius suggestion from Peter.

psteinfeld force-pushed the ps-stmt-stack branch from 30ab8e6 to be2a03e Compare September 10, 2019 22:02

psteinfeld requested a review from klausler September 10, 2019 22:04

klausler approved these changes Sep 10, 2019

View reviewed changes

tskeith approved these changes Sep 10, 2019

View reviewed changes

psteinfeld merged commit 2fc28c7 into master Sep 10, 2019

psteinfeld deleted the ps-stmt-stack branch September 10, 2019 22:27

psteinfeld mentioned this pull request Dec 11, 2019

No error message for stand-alone CYCLE statement #626

Closed

Changes to add an executable construct stack #686

Changes to add an executable construct stack #686

Conversation

psteinfeld commented Aug 26, 2019

ichoyjx commented Aug 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psteinfeld commented Aug 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vjayathirtha-nv Sep 10, 2019 • edited

Choose a reason for hiding this comment

vjayathirtha-nv Sep 10, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vjayathirtha-nv Sep 4, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psteinfeld commented Sep 10, 2019

Choose a reason for hiding this comment

klausler Sep 10, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psteinfeld commented Sep 10, 2019

vjayathirtha-nv Sep 10, 2019 •

edited

vjayathirtha-nv Sep 10, 2019 •

edited

vjayathirtha-nv Sep 4, 2019 •

edited

klausler Sep 10, 2019 •

edited