[PATCH] D102210: [flang] Allow large and erroneous ac-implied-do's

[Pete Steinfeld via Phabricator via llvm-commits]

PeteSteinfeld added a comment.

Thanks for the feedback, Jean!



================
Comment at: flang/include/flang/Evaluate/shape.h:187
+          // large we can't evalute it without overflowing the stack.
+          result = Fold(*context_, std::move(result));
+        }
----------------
jeanPerier wrote:
> Are they not cases where the additions cannot be folded and the array constructor could still grow big ?
> If so, then we will still eventually hit the overflow, and every Fold might actually get expensive.
> 
> Maybe `GetArrayConstructorExtent` should give up if the non constant part grows too much (this could be done by keeping constants `n` and non constants `n` in a different accumulators for which the number of operands would be tracked, and  `GetArrayConstructorExtent` would return nullopt after a certain threshold is reached).
> 
> I am not sure if there are actually cases where non constant `n` could still lead to an overflow (I suppose ac-implied-do containing such dynamic extents expression would not be unrolled). So what you have may be enough until we hit a scenario where more is needed here.
I don't believe that there are cases where the additions cannot be folded.  One of the conditions for folding is that the initial, final, and stride expressions of the ac-implied-do are all constants.  The large extent expression that gets produced is based on these constant values.  So I don't believe that, with this change, it's possible to create an extent expression that will overflow the stack no matter how large the unrolled list of values is.  Note that the values in the unrolled list are not necessarily constant and are not used in the folding.  It's only the extent expression.

I like your suggestion of giving up on expression processing if an expression grows to large.  I think that this should be done generally for all expression processing.  I'm working on a more general way to do this.


================
Comment at: flang/lib/Parser/message.cpp:330
   for (const Message *msg : sorted) {
+    if (lastMsg && *msg == *lastMsg) {
+      // Don't emit two identical messages for the same location
----------------
jeanPerier wrote:
> I am not opposed to having this here. I am wondering if the places that are raising the same messages over and over should not also be  be prevented from doing so. Maybe some checkers in ArrayConstructors should stop after x errors to avoid wasting time and filling `messages_`.
> Again, if you considered it and it turned out unpractical, what you have here already looks like an improvement to me, and it had the benefit to clean some other small error duplication cases.
Here's an example of code that will produce many messages:
```
  integer(foo),parameter :: jval4(*) = (/ (0_foo,ii=1,10) /)
```

Without this change, the compiler will produce 10 identical messages for the one erroneous instance of "0_foo":
```
./j1.f90:2:46: error: Must be a constant value
    integer(foo),parameter :: jval4(*) = (/ (0_foo,ii=1,10) /)
                                               ^^^
./j1.f90:2:46: error: Must be a constant value
    integer(foo),parameter :: jval4(*) = (/ (0_foo,ii=1,10) /)
                                               ^^^
    ...
```

Basing the elimination of messages on the number of messages seems incorrect to me.  Rather, we should only produce one message for each group of unrolled messages.  It might be possible to do that.  But the current code solves the same problem in a more general way.  Let me know if you disagree.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102210