[PATCH] D77821: [Flang][OpenMP] Avoid abort when collapse clause value is negative

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D77821#1981149 <https://reviews.llvm.org/D77821#1981149>, @klausler wrote:

> In D77821#1979385 <https://reviews.llvm.org/D77821#1979385>, @jdoerfert wrote:
>
> > In D77821#1975101 <https://reviews.llvm.org/D77821#1975101>, @klausler wrote:
> >
> > > > As far as I can tell, the collapse argument is a `scalarIntConstantExpr` to the parser. So conceptually one could make it a `scalarPositiveIntConstantExpr` and issue an error early.
> > >
> > > Fortran has scalar-int-constant-expr but not scalar-positive-int-constant-expr as nonterminals.
> > >
> > > The f18 parser doesn't do any semantic checking in any case.
> >
> >
> > This is arguably not true. The OpenMP grammar does not restrict the the collapse value to be a constant, the description does *in the same sentence* it is restricted to be positive.
> >  If you would not perform any semantic check you would allow an arbitrary expression here and check it later in sema. My argument now is that this does make as much sense as checking that it is a constant but not if it is positive.
> >
> > > The various constrained nonterminals of Fortran are represented in the parse tree and checked later in expression semantics.
> >
> > But apparently not before you access/use input not validated yet. Take the TODO that I quoted (which is above the method that now duplicates the semantic check for the collapse clause value).
> >  It already noted that there is an implicit dependence here. Resolving that by duplicating checks seems conceptually the wrong approach. (You also now have signed values floating around even though you want conceptually unsigned values.)
>
>
> I can't figure out what you're trying to express,


Hm, unsure what was confusing in my response. Maybe you say which of these is unclear or if it something else entirely:

- My concerns with this kind of "fix", e.g., how it duplicates the verification logic and ties into the `TODO` above he function.
- My suggested alternative "fix", that is parsing positive-constants.
- My argument that the line drawn in the current parser is arbitrary, e.g., it cannot be justified with the OpenMP standard, and therefor not a good reason to keep it this way.



> or what part of my comment was "arguably not true".

The part I quoted. To make it clear, this part: *The f18 parser doesn't do any semantic checking in any case.*
I then explained why I think that with respect to the OpenMP standard. From your answer below I sense you might
draw a line between parsing as building (an annotated) parse tree and parsing as all the effects of the code in the
flang/parser folder. FWIW, I was talking about the latter.

> The Fortran 2018 grammar has some conventions that allow the names of nonterminals to be qualified with semantic restriction prefixes (4.1.3 for "scalar").  So a "scalar-int-constant-expr" is an "int-constant-expr" (R1031) that is semantically constrained to be scalar and constant (C1031) in the Fortran sense of a "constant expression" (10.1.12) and integer (C1008).  The f18 parse tree represents constraints like this with template classes (`Scalar<>`, `Integer<>`, &c.) that wrap a subtree with a representation of a check that is applied after parsing during the semantic analysis and validation of expressions and variable references.  The expressions and variable references in the parse tree are analyzed and validated after parsing has recognized a syntactically valid program, name resolution has created the symbols and scopes and types, and any necessary corrections to ambiguous parses have been applied to the parse tree.  The semantic analysis of expressions and variables generates (in the absence of errors) a strongly-typed internal representation of those objects, and attaches those representations to the parse tree so that the later semantic passes have access to them.  All compiler messages from all phases get buffered, then sorted in source position order and emitted later after all messages have been created.

Thank you for the detailed explanation. Unfortunately, now I am the one unsure what you want to tell me.

I never argued about the Fortran grammar but the OpenMP one, this is about OpenMP parsing/sema after all and the Fortran grammar does not mention the collapse clause (I guess at least). I also did not say you should do anything different *except* what is basically exposed with this patch. That is, `PrivatizeAssociatedLoopIndex` is run on an AST nodes in which the associated loops has an invalid value. This patch "fixes" `PrivatizeAssociatedLoopIndex` so it can cope with such invalid input. My point is that this duplicates the burden of validating the associated loops count.

---

To recap my argument:

As far as I can tell, when `PrivatizeAssociatedLoopIndex` is entered the `collapse` clause argument has been evaluated already (, which is why the test failed). If that argument was defined as `scalarPositiveIntConstantExpr` instead of a `scalarIntConstantExpr` the pre-condition mentioned in the TODO above `PrivatizeAssociatedLoopIndex` would be satisfied by construction and we would not need to worry about invalid inputs here or elsewhere for that matter. The OpenMP standard defines the argument of a `collapse` clause as positive constant. Expecting a positive constant already during parsing (=the code in flang/parser) seems more reasonable to me as parsing a constant (which is also only taken from the OpenMP standard) and then verifying the value in a second step.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77821