[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
llvm-commits at lists.llvm.org
Mon Sep 27 21:00:46 PDT 2021
NimishMishra added inline comments.
================
Comment at: flang/lib/Semantics/check-directive-structure.h:19
#include <unordered_map>
-
namespace Fortran::semantics {
----------------
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.
================
Comment at: flang/lib/Semantics/check-directive-structure.h:121
const char *stmt, const parser::Name &stmtName) {
- const ConstructStack &stack{context_.constructStack()};
+ const ConstructStack stack{context_.constructStack()};
for (auto iter{stack.cend()}; iter-- != stack.cbegin();) {
----------------
kiranchandramohan wrote:
> Wouldn't the removal of the reference result in a copy? Is that the intention here?
This is a slip from my end. I did not intend it. Will add the reference back
================
Comment at: flang/lib/Semantics/check-directive-structure.h:134
+ bool CheckForRequiredConstruct(const ConstructNode &construct) const {
+ return std::visit(common::visitors{
----------------
kiranchandramohan wrote:
> CheckForRequiredConstruct -> CheckForRequiredDoConstruct or CheckForDoConstruct
Carrying out this discussion in the other comment thread as well: on there being a counter instead of a stack
================
Comment at: flang/lib/Semantics/check-directive-structure.h:160
+ // encountering an OpenMP/OpenACC directive
+ ConstructStack privateConstructStack;
SemanticsContext &context_;
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D108904/new/
https://reviews.llvm.org/D108904
More information about the llvm-commits
mailing list