[flang-commits] [flang] [flang] Fix sequence association for PARAMETER array elements (PR #187348)

[via flang-commits]

https://github.com/jeanPerier commented:

I am unsure we should support this because I think the cost of fully supporting this is to move constants out of read only memory.

The first thing is that it is a bit unclear to me if sequence association truly applies designator with a named constant base since these are not variables. While I fully agree 15.5.2.12 is not directly ruling out named constants, it says: _"if the actual argument is an array element designator of a simply contiguous array, the element sequence consists of that array element and each element that follows it in array element order"_ and "simply contiguous array" are only defined for variables in 9.5.4. So, one could interpret that constant designator should fall into the first or last bullet point.

But that is a very debatable reading and I can see why some people would expect it to be possible and allowed.

Which brings me to why I think your patch is not sufficient to support the feature and we probably need to resort to moving constant out of read only memory.

The first issue with the patch that I see is that it relies on having identify that the dummy is an array, but with because of implicit interfaces, the very fact that sequence association may occur may be unknown to the compiler.

Take the following code with foo being defined in another file:
```
  integer, parameter :: x(10) = [(i, i=1,10)]
  call foo(x(1))
end
```

```
subroutine foo(x)
  integer :: x(10)
  print * x
end subroutine
```

So this means that your patch should not consider anything from the dummy and skip the copy of all scalar parameters. This is not a blocker issue, it just means you have to skip the copy in more cases. Which leads to the second point, which is the purpose of the copy.

The second issue I see is that skipping the copy will lead to crashes if any of the sequence associated parameters are later used in a copy in-out scenario. Take the following example that is valid assuming sequence association of PARAMETERs is expected:

```
subroutine foo(x)
  integer :: x(10)
  call bar(x(1:10:2))
end subroutine

subroutine bar(x)
  integer :: x(5)
  print *, x
end subroutine

  interface
    subroutine foo(x)
      integer :: x(10)
    end subroutine
  end interface
  integer, parameter :: x(10) = [(i, i=1,10)]
  call foo(x(1))
end
```

It will still not behave as a user expecting sequence association of parameters to work  would think (it will segfault).

The reason a copy is introduced is because our copy-out implementation is unconditionally copying back the buffer back if a buffer was created, even when the call to `bar` did not modify the buffer. So if the storage for the variable being copied out to is in read only memory, the runtime will segfault.

So if we want to correctly support this feature, we either need to move constants out of read only memory, or we need to somehow change copyout implementations from copying back to read only data.

The reason we place constants in read only memory is both to help LLVM optimizations by telling them this memory is invariant, and to catch attempt by a user to modify them. Although, that last point is actually made a bit moot by the very fact that we are making copies before passing constants (which means a user can redefined such argument while they should not and the code will not trap since it is OK to modify the copy).

We could have also a copy-out implementation that compares the input to the buffer and only copies if anything changed. This would also solve the issue, but may impact performance because of the extra copy (although this extra cost could be balanced by all the cases where no write is done because the data is actually not modified). Both the runtime and inlined version would need to be modified. This would have the benefit of allowing us to actually catch (with a segfault) definition of dummy arguments associated to parameters.

https://github.com/llvm/llvm-project/pull/187348