[flang-commits] [flang] [Flang][OpenMP] Fix implicit symbol resolution for USE-renamed arrays (PR #189215)

[Kiran Chandramohan via flang-commits]

kiranchandramohan wrote:

> @kiranchandramohan, about the assert, `DeclareNewAccessEntity()` was originally part of `DeclareAccessEntity()`. I've moved it to a new function to be able to use a `const Symbol &object`, since `DeclareAccessEntity()` modifies the object if it's owned by the current scope. That's why I added the assert. But since we always pass the scope to `MakeAssocSymbol()`, this assert can probably be removed. In this sense `assert(object.owner() != scope)` could make more sense, since in this case we would not be creating a new symbol, but just modifying the existing one. But I guess it's better to just remove the assert, to avoid surprises.
> 
> I'm not sure about using `symbol` instead of `&symbol->GetUltimate()` in `CreateImplicitSymbols()`. My understanding is that `symbol` can be in any scope, depending on the parse tree node that is being processed, while a few places, such as `GetSymbolDSA()`, assume that an OpenMP host association will point to the symbol in the nested scope, even though there are exceptions, such as predetermined loop iteration variables, that can point directly to the ultimate symbol, IIRC.
> 
> ~To handle USE-rename, wouldn't it be possible to go through the use details to find the "closest" symbol and then create an association with it, but using the new name? If it points directly to a module symbol, that should be the correct one. If it was privatized, we should find an association before the use statement, unless I'm missing something.~
> 
> The changes in this PR looks good to me, as `CreateImplicitSymbols()` always starts from the outermost context, where no privatization should have happened yet, and thus the ultimate symbol should be the correct one to use.
> 
> > This change will make the symbol in the OpenMP region refer to the correct symbol in the module. What I was worried about was whether this will cause a problem if the array is modified (via an allocation) just before the OpenMP region, will pointing to the symbol (via host-association) in the module cause it get the array with the previous allocation?
> 
> I'm not sure if I understood this case. Do you have something like the following in mind?
> 
> ```fortran-modern
> module mod1
>   implicit none
>   real(8), allocatable :: ary(:,:,:)
> end module mod1
> 
> module mod2
>   implicit none
>   real(8), allocatable :: ary(:,:,:,:,:)
> end module mod2
> 
> program test
>   use mod1, only: s_ary => ary
>   use mod2, only: ary
>   implicit none
>   integer(4) :: i
> 
>   allocate(s_ary(10,1,1))
>   allocate(ary(10,1,1,1,1))
> 
>   s_ary = 0
>   ary = 2
> 
>   !$omp parallel !firstprivate(ary)
>     !$omp do
>     do i = 1, 10
>       s_ary(i,1,1) = ary(i,1,1,1,1)
>     end do
>   !$omp end parallel
> 
>   print *, s_ary
> end program
> ```
> 
> This works correctly with this change, with or without the `firstprivate` clause.



> @kiranchandramohan, about the assert, `DeclareNewAccessEntity()` was originally part of `DeclareAccessEntity()`. I've moved it to a new function to be able to use a `const Symbol &object`, since `DeclareAccessEntity()` modifies the object if it's owned by the current scope. That's why I added the assert. But since we always pass the scope to `MakeAssocSymbol()`, this assert can probably be removed. In this sense `assert(object.owner() != scope)` could make more sense, since in this case we would not be creating a new symbol, but just modifying the existing one. But I guess it's better to just remove the assert, to avoid surprises.
> 
> I'm not sure about using `symbol` instead of `&symbol->GetUltimate()` in `CreateImplicitSymbols()`. My understanding is that `symbol` can be in any scope, depending on the parse tree node that is being processed, while a few places, such as `GetSymbolDSA()`, assume that an OpenMP host association will point to the symbol in the nested scope, even though there are exceptions, such as predetermined loop iteration variables, that can point directly to the ultimate symbol, IIRC.
> 
> ~To handle USE-rename, wouldn't it be possible to go through the use details to find the "closest" symbol and then create an association with it, but using the new name? If it points directly to a module symbol, that should be the correct one. If it was privatized, we should find an association before the use statement, unless I'm missing something.~
> 
> The changes in this PR looks good to me, as `CreateImplicitSymbols()` always starts from the outermost context, where no privatization should have happened yet, and thus the ultimate symbol should be the correct one to use.
> 
> > This change will make the symbol in the OpenMP region refer to the correct symbol in the module. What I was worried about was whether this will cause a problem if the array is modified (via an allocation) just before the OpenMP region, will pointing to the symbol (via host-association) in the module cause it get the array with the previous allocation?
> 
> I'm not sure if I understood this case. Do you have something like the following in mind?
> 
> ```fortran-modern
> module mod1
>   implicit none
>   real(8), allocatable :: ary(:,:,:)
> end module mod1
> 
> module mod2
>   implicit none
>   real(8), allocatable :: ary(:,:,:,:,:)
> end module mod2
> 
> program test
>   use mod1, only: s_ary => ary
>   use mod2, only: ary
>   implicit none
>   integer(4) :: i
> 
>   allocate(s_ary(10,1,1))
>   allocate(ary(10,1,1,1,1))
> 
>   s_ary = 0
>   ary = 2
> 
>   !$omp parallel !firstprivate(ary)
>     !$omp do
>     do i = 1, 10
>       s_ary(i,1,1) = ary(i,1,1,1,1)
>     end do
>   !$omp end parallel
> 
>   print *, s_ary
> end program
> ```
> 
> This works correctly with this change, with or without the `firstprivate` clause.

Thanks @luporl for the explanation.

Yes, the example provided is the one I had in mind. An assignment can also change the allocation. As long as the copies in the OpenMP region are using the updated assignment it is fine. I am assuming lowering is handling all these due to scoped Symbol Table.

Feel free to go ahead with the change.

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