[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#1982275 <https://reviews.llvm.org/D77821#1982275>, @klausler wrote:

> In D77821#1982012 <https://reviews.llvm.org/D77821#1982012>, @jdoerfert wrote:
>
> > 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.
>
>
> The code that was moved into the flang/lib/Parser directory comprises preprocessing, prescanning into a normalized source form, parsing proper with construction of a parse tree, message formatting and source provenance, and utilities for formatting the parse tree in various ways.  None of the code in flang/lib/Parser performs semantic analysis.  It is not possible to validate any semantic constraints in the parser, including the existing constraints (scalar, integer, constant, default character, logical, &c.) and the new one you suggest, because the parser has no symbol table and cannot evaluate expressions.  All of that semantic checking is performed by code that is not located in the flang/lib/Parser and that runs after a successful parse of the entire source file is complete.


You seem to not address any of my concerns but eager to lecture me about something different. This is not helping at all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77821