[PATCH] D108904: [flang][OpenMP] Added semantic checks for sections (associated section(s) should be structured block(s)) and simd constructs (associated loop(s) should be structured block(s))

[Nimish Mishra via Phabricator via llvm-commits]

NimishMishra marked an inline comment as done.
NimishMishra added inline comments.


================
Comment at: flang/lib/Semantics/check-directive-structure.h:19
 #include <unordered_map>
-
 namespace Fortran::semantics {
----------------
NimishMishra wrote:
> kiranchandramohan wrote:
> > Nit: Accidentally removed?
> No I don't think so. I did add //#include<assert.h>// when I included that assert statement. When that statement was removed, this include was also removed. I think a space may have stayed there, and got removed this time during formatting.
Please disregard the above comment. This was due to my editor's configuration. I will revert this space back as it was before.


================
Comment at: flang/lib/Semantics/check-directive-structure.h:148
+      // found an enclosing looping construct for the unlabelled EXIT/CYCLE
+      if (CheckForRequiredConstruct(construct)) {
+        return;
----------------
kiranchandramohan wrote:
> This function can be a lambda.
With the counter approach, I think this won't be necessary anymore.


================
Comment at: flang/lib/Semantics/check-directive-structure.h:160
+  // encountering an OpenMP/OpenACC directive
+  ConstructStack privateConstructStack;
   SemanticsContext &context_;
----------------
kiranchandramohan wrote:
> NimishMishra wrote:
> > kiranchandramohan wrote:
> > > Looking at this again, do we need a construct stack? Can we just maintain a DoConstruct counter (which increments on entering (or Pre) a Do and decrements on leaving (or Post) a Do)? And if the DoConstruct counter value is greater than 0 then we are inside a DoConstruct, else outside.
> > Yes it can be. However, note that this //class NoBranchingEnforce// is used by a //parser::Walk// within //CheckNoBranching// function whose aim is to check non-essential branching within OpenMP/OpenACC constructs.
> > 
> > Do you think, in foresight, that this private stack approach would be better than the counter approach? Just in case there may come up a use for some other construct as we expand our OpenMP/OpenACC support. 
> > 
> > Similar thought I had for the introduced //CheckForRequiredConstruct//. As we expand, it could serve as the point of call from various other functions/places aiding them to make a decision on when to throw the illegal branching error.
> In the past, there have been instances where we have replaced with a more general data-structure. If you can point out a particular case where this will be required then let us keep the construct stack and add a comment saying this will be useful when we implement that case. Otherwise, let us replace it with a counter.
> 
> Can you quickly glance through the standard and have a quick check whether there are such checks required? From the top of my head, I don't see anything but I could have missed something.
I searched through the standard. As far as I can see, I found a few other examples where this could be useful but none of them were without //DoConstruct//. 

So I think I will remove this stack approach now and replace it with a private counter.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108904/new/

https://reviews.llvm.org/D108904