[PATCH] Fortran: Added support for locality specs in DO CONCURRENT (Fortran 2018/23)

[Tobias Burnus]

Hi,

On 9/25/24 3:18 AM, Andre Vehreschild wrote:

>> @@ -3089,7 +3099,15 @@ typedef struct gfc_code
>>       gfc_inquire *inquire;
>>       gfc_wait *wait;
>>       gfc_dt *dt;
>> -    gfc_forall_iterator *forall_iterator;
>> +
>> +    struct
>> +    {
>> +      gfc_forall_iterator *forall_iterator;
>> +      gfc_expr_list *locality[LOCALITY_NUM];
>> +      bool default_none;
>> +    }
>> +    concur;
>
> I am more than unhappy about that construct. Because every concurrent 
> loop has
> a forall_iterator, but not every forall_iterator is a concurrent loop. I
> therefore propose to move the forall_iterator out of the struct and 
> only have
> the concurrent specific elements in the struct. This would also reduce 
> the
> changes significantly. 

First, regarding the naming, Fortran 2018 had:

FORALL forall-header

[do-stmt …]
CONCURRENT forall-header

Thus, both do-concurrent and forall used both a forall-header.

Since Fortran 2023, there is now:
    FORALL concurrent-header
etc.

Thus, in Fortran 2024 both use a concurrent-header.

* * *

On the technical side:

   DO CONCURRENT (i = 1:5) mask(.true.) local(x) default(none)

This has two parts:

* A forall_iterator: 'i = 1:5' and one 'mask' expression
* A locality spec: default(none) local(x)

The forall-header is saved on mainline as:

   new_st.expr1 = mask;
   new_st.ext.forall_iterator = head;

Since Fortran 2018 (and for this patch) we additionally have to save
for 'do concurrent' somehow a boolean ('default(none)') and a list of
symbols (with knowledge about in which locality they appeared).

Storing the forall-header iterator in <gfc_code>.ext.forall_iterator
sounds fine, but where to put locality other data?

The most sensible place is to put it also into <gfc_code>.ext, but as
the latter is a union, we need to ensure that both the iterator *and*
the locality data is available. - Thus, we create a struct for
'do concurrent'. But as the forall-header / concurrent-header is identical,
it makes sense to also use the same struct for FORALL and not to duplicate
code here.

IMHO the current code is fine.

* * *

On 1/7/25 12:06 PM, Jerry D wrote:

> cannot understand why moving the forall_iterator from the 
> sub-structure 'concur' back to where it was at the 'ext' sub-structure 
> of typedef struct gfc_code. 'ext' is a union. I suspected there is an 
> overlap going on there such that something is getting overwritten or 
> optimized away.

Well, as mentioned, a DO CONCURRENT can have both. Assume a simple do-concurrent loop:
   DO, concurrent (I = 0:4)

This will fill code.ext.forall_iterator, which is fine. When this being resolved
in resolve.cc, for checking the iterator, the access goes to
   code.ext.forall_iterator
which is fine - this will work for the example above.

As next step, the locality is checked, accessing
   code.ext.concurr.*

but that variable shares the memory (union!) with code.ext.forall_iterator. Thus,
accessing locality[0] will be identical to
   static_cast<gfc_expr_list *>(code.ext.forall_iterator)
and this has a very high chance to crash.

When using 'do concurrent(i=0:5) local(x)', the code.ext.forall_iterator
would be already overridden during parsing.

* * *

I hope that helps!

Tobias