Re: [PATCH] tree-parloops: Fix iteration-private memory decl hoisting [PR83064]

On 17/07/26 15:58, Richard Biener wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Fri, Jul 17, 2026 at 6:47 AM <[email protected]> wrote:
>>
>> From: Dhruv Chawla <[email protected]>
>>
>> Leave iteration-private decls in place instead of hoisting them, so
>> move_sese_region_to_fn gives each thread function its own copy. This fixes the
>> testcase mentioned in PR83064, which involves a stack array used as a callee
>> return slot getting shared across multiple threads leading to a data race and
>> therefore multiple writes to the same location.
>>
>> This is a conservative way to solve this issue, by assuming that any memory
>> decl not referenced outside the loop is private to the iteration. It
>> would still be possible to hoist such memory if it was provable that the
>> same location would not be written across multiple threads, but that
>> analysis would be much more complicated and potentially unlikely to have
>> much benefit. Globals are always hoisted.
>>
>> The gather_escaping_decls function collects those that escape before the loop
>> is duplicated by versioning or the exit-first transformation, as that would
>> otherwise make a private decl look like it escapes.
>>
>> This is an alternate attempt to fix PR125717, as this brings back the
>> performance on the 800.pot3d_s benchmark to the expected levels. The previous
>> attempt was at
>> https://gcc.gnu.org/pipermail/gcc-patches/2026-June/720592.html.
>>
>> Bootstrapped and regtested on aarch64-linux-gnu.
>>
>> Signed-off-by: Dhruv Chawla <[email protected]>
>>
>>          PR tree-optimization/83064
>>
>> gcc/fortran/ChangeLog:
>>
>>          * trans-stmt.cc (gfc_trans_forall_loop): Emit
>>          annot_expr_parallel_kind for do-concurrent loops.
>>
>> gcc/ChangeLog:
>>
>>          * tree-parloops.cc (struct elv_data): Add escaping hash_set.
>>          (eliminate_local_variables_1): Only hoist escaping local
>>          variables.
>>          (eliminate_local_variables_stmt): Pass the escaping set through
>>          to eliminate_local_variables_1.
>>          (record_memory_decl): New function.
>>          (gather_escaping_decls): Likewise.
>>          (eliminate_local_variables): Pass the escaping set through to
>>          eliminate_local_variables_stmt.
>>          (gen_parallel_loop): Compute escaping set via
>>          gather_escaping_decls and pass to
>>          eliminate_local_variables.
>>
>> gcc/testsuite/ChangeLog:
>>
>>          * gfortran.dg/do_concurrent_6.f90: Update to handle parallel
>>          annotation.
>>          * gfortran.dg/do_concurrent_7.f90: Likewise.
>>          * gfortran.dg/do_concurrent_16.f90: New test.
>> ---
>>   gcc/fortran/trans-stmt.cc                     | 12 +--
>>   .../gfortran.dg/do_concurrent_16.f90          | 68 +++++++++++++++
>>   gcc/testsuite/gfortran.dg/do_concurrent_6.f90 |  2 +-
>>   gcc/testsuite/gfortran.dg/do_concurrent_7.f90 |  6 +-
>>   gcc/tree-parloops.cc                          | 82 +++++++++++++++++--
>>   5 files changed, 154 insertions(+), 16 deletions(-)
>>   create mode 100644 gcc/testsuite/gfortran.dg/do_concurrent_16.f90
>>
>> diff --git a/gcc/fortran/trans-stmt.cc b/gcc/fortran/trans-stmt.cc
>> index 526f42febf7..3c9ebdd0274 100644
>> --- a/gcc/fortran/trans-stmt.cc
>> +++ b/gcc/fortran/trans-stmt.cc
>> @@ -4349,13 +4349,13 @@ gfc_trans_forall_loop (forall_info *forall_tmp, tree body,
>>         cond = fold_build2_loc (input_location, LE_EXPR, logical_type_node,
>>                                count, build_int_cst (TREE_TYPE (count), 0));
>>
>> -      /* PR 83064 means that we cannot use annot_expr_parallel_kind until
>> -       the autoparallelizer can handle this.  */
>> +      annot_expr_kind kind = forall_tmp->do_concurrent
>> +                              ? annot_expr_parallel_kind
>> +                              : annot_expr_ivdep_kind;
>>         if (forall_tmp->do_concurrent || iter->annot.ivdep)
>> -       cond = build3 (ANNOTATE_EXPR, TREE_TYPE (cond), cond,
>> -                      build_int_cst (integer_type_node,
>> -                                     annot_expr_ivdep_kind),
>> -                      integer_zero_node);
>> +       cond
>> +         = build3 (ANNOTATE_EXPR, TREE_TYPE (cond), cond,
>> +                   build_int_cst (integer_type_node, kind), integer_zero_node);
>>
>>         if (iter->annot.unroll && cond != error_mark_node)
>>          cond = build3 (ANNOTATE_EXPR, TREE_TYPE (cond), cond,
>> diff --git a/gcc/testsuite/gfortran.dg/do_concurrent_16.f90 b/gcc/testsuite/gfortran.dg/do_concurrent_16.f90
>> new file mode 100644
>> index 00000000000..a3df15815bb
>> --- /dev/null
>> +++ b/gcc/testsuite/gfortran.dg/do_concurrent_16.f90
>> @@ -0,0 +1,68 @@
>> +! { dg-do run }
>> +! { dg-require-effective-target pthread }
>> +! { dg-additional-options "-O2 -ftree-parallelize-loops=2 -fdump-tree-parloops2-details" }
>> +!
>> +! PR tree-optimization/83064
>> +!
>> +! Run two loops, one parallelized via do-concurrent and one serial. Compare
>> +! their results to make sure that the loop that is parallelized computes the
>> +! correct and expected result.
>> +
>> +program main
>> +    use, intrinsic :: iso_fortran_env
>> +    implicit none
>> +
>> +    integer, parameter :: nsplit = 4
>> +    integer(int64), parameter :: ne = 100000
>> +    integer(int64) :: stride, low(nsplit), high(nsplit), edof(ne), i
>> +    real(real64), dimension(nsplit) :: pi
>> +    real(real64) :: par, ser
>> +
>> +    edof(1::4) = 1
>> +    edof(2::4) = 2
>> +    edof(3::4) = 3
>> +    edof(4::4) = 4
>> +
>> +    stride = ceiling(real(ne)/nsplit)
>> +    do i = 1, nsplit
>> +        high(i) = stride*i
>> +    end do
>> +    do i = 2, nsplit
>> +        low(i) = high(i-1) + 1
>> +    end do
>> +    low(1) = 1
>> +    high(nsplit) = ne
>> +
>> +    ! Parallel loop.
>> +    pi = 0
>> +    do concurrent (i = 1:nsplit)
>> +        pi(i) = sum(compute( low(i), high(i) ))
>> +    end do
>> +    par = 4*sum(pi)
>> +
>> +    ! Serial loop as a reference.
>> +    pi = 0
>> +    do i = 1, nsplit
>> +        pi(i) = sum(compute( low(i), high(i) ))
>> +    end do
>> +    ser = 4*sum(pi)
>> +
>> +    if (abs(par - ser) > 1.0e-9_real64 * abs(ser)) stop 1
>> +
>> +contains
>> +
>> +    pure function compute( low, high ) result( tmp )
>> +        integer(int64), intent(in) :: low, high
>> +        real(real64), dimension(nsplit) :: tmp
>> +        integer(int64) :: j, k
>> +
>> +        tmp = 0
>> +        do j = low, high
>> +            k = edof(j)
>> +            tmp(k) = tmp(k) + (-1.0_real64)**(j+1) / real( 2*j-1 )
>> +        end do
>> +    end function
>> +
>> +end program main
>> +
>> +! { dg-final { scan-tree-dump "parallelizing \[^\n\r\]*loop" "parloops2" } }
>> diff --git a/gcc/testsuite/gfortran.dg/do_concurrent_6.f90 b/gcc/testsuite/gfortran.dg/do_concurrent_6.f90
>> index 9585a9f96f4..5320fd108a1 100644
>> --- a/gcc/testsuite/gfortran.dg/do_concurrent_6.f90
>> +++ b/gcc/testsuite/gfortran.dg/do_concurrent_6.f90
>> @@ -10,4 +10,4 @@ program main
>>     print *,sum(a)
>>   end program main
>>
>> -! { dg-final { scan-tree-dump-times "ivdep" 1 "original" } }
>> +! { dg-final { scan-tree-dump-times "parallel" 1 "original" } }
>> diff --git a/gcc/testsuite/gfortran.dg/do_concurrent_7.f90 b/gcc/testsuite/gfortran.dg/do_concurrent_7.f90
>> index 604f6712d05..c4c73094960 100644
>> --- a/gcc/testsuite/gfortran.dg/do_concurrent_7.f90
>> +++ b/gcc/testsuite/gfortran.dg/do_concurrent_7.f90
>> @@ -21,6 +21,6 @@ program dc
>>     end do
>>   end program
>>
>> -! { dg-final { scan-tree-dump "ANNOTATE_EXPR .* ivdep>, vector" "original" } }
>> -! { dg-final { scan-tree-dump "ANNOTATE_EXPR .* ivdep>, no-vector" "original" } }
>> -! { dg-final { scan-tree-dump "ANNOTATE_EXPR .* ivdep>, unroll 4>, no-vector" "original" } }
>> +! { dg-final { scan-tree-dump "ANNOTATE_EXPR .* parallel>, vector" "original" } }
>> +! { dg-final { scan-tree-dump "ANNOTATE_EXPR .* parallel>, no-vector" "original" } }
>> +! { dg-final { scan-tree-dump "ANNOTATE_EXPR .* parallel>, unroll 4>, no-vector" "original" } }
>> diff --git a/gcc/tree-parloops.cc b/gcc/tree-parloops.cc
>> index 1da23f8d3c8..c16ebc7ba76 100644
>> --- a/gcc/tree-parloops.cc
>> +++ b/gcc/tree-parloops.cc
>> @@ -1151,6 +1151,7 @@ struct elv_data
>>     struct walk_stmt_info info;
>>     edge entry;
>>     int_tree_htab_type *decl_address;
>> +  hash_set<tree> *escaping;
>>     gimple_stmt_iterator *gsi;
>>     bool changed;
>>     bool reset;
>> @@ -1175,6 +1176,9 @@ eliminate_local_variables_1 (tree *tp, int *walk_subtrees, void *data)
>>         if (!SSA_VAR_P (t) || DECL_EXTERNAL (t))
>>          return NULL_TREE;
>>
>> +      if (dta->escaping && !is_global_var (t) && !dta->escaping->contains (t))
>> +       return NULL_TREE;
>> +
>>         type = TREE_TYPE (t);
>>         addr_type = build_pointer_type (type);
>>         addr = take_address_of (t, addr_type, dta->entry, dta->decl_address,
>> @@ -1213,6 +1217,10 @@ eliminate_local_variables_1 (tree *tp, int *walk_subtrees, void *data)
>>         if (!var || !SSA_VAR_P (var) || DECL_EXTERNAL (var))
>>          return NULL_TREE;
>>
>> +      if (dta->escaping && !is_global_var (var)
>> +         && !dta->escaping->contains (var))
>> +       return NULL_TREE;
>> +
>>         addr_type = TREE_TYPE (t);
>>         addr = take_address_of (obj, addr_type, dta->entry, dta->decl_address,
>>                                dta->gsi);
>> @@ -1240,7 +1248,8 @@ eliminate_local_variables_1 (tree *tp, int *walk_subtrees, void *data)
>>
>>   static void
>>   eliminate_local_variables_stmt (edge entry, gimple_stmt_iterator *gsi,
>> -                               int_tree_htab_type *decl_address)
>> +                               int_tree_htab_type *decl_address,
>> +                               hash_set<tree> *escaping)
>>   {
>>     struct elv_data dta;
>>     gimple *stmt = gsi_stmt (*gsi);
>> @@ -1248,6 +1257,7 @@ eliminate_local_variables_stmt (edge entry, gimple_stmt_iterator *gsi,
>>     memset (&dta.info, '\0', sizeof (dta.info));
>>     dta.entry = entry;
>>     dta.decl_address = decl_address;
>> +  dta.escaping = escaping;
>>     dta.changed = false;
>>     dta.reset = false;
>>
>> @@ -1279,6 +1289,53 @@ eliminate_local_variables_stmt (edge entry, gimple_stmt_iterator *gsi,
>>       update_stmt (stmt);
>>   }
>>
>> +/* Callback for walk_stmt_load_store_addr_ops to record into DATA any memory
>> +   decls referenced by the gimple stmt.  */
>> +
>> +static bool
>> +record_memory_decl (gimple *, tree base, tree, void *data)
>> +{
>> +  hash_set<tree> *decls = (hash_set<tree> *) data;
>> +  base = get_base_address (base);
>> +  if (DECL_P (base) && SSA_VAR_P (base) && !is_global_var (base)
>> +      && !DECL_EXTERNAL (base))

Hi Richi,

> 
> This looks like you want
> 
>      if (VAR_P (base) && auto_var_in_fn_p (base, current_function_decl))

Thanks, that is much cleaner.

> 
>> +    decls->add (base);
>> +  return false;
>> +}
>> +
>> +/* Collect into ESCAPING the memory decls referenced outside LOOP.  Must run
>> +   before LOOP is duplicated as a duplicated body would otherwise make a private
>> +   decl look like it escapes.  */
>> +
>> +static void
>> +gather_escaping_decls (class loop *loop, hash_set<tree> *escaping)
>> +{
>> +  basic_block *bbs = get_loop_body (loop);
>> +  unsigned i;
>> +
>> +  auto_bitmap in_loop;
>> +  for (i = 0; i < loop->num_nodes; i++)
>> +    bitmap_set_bit (in_loop, bbs[i]->index);
>> +  free (bbs);
>> +
>> +  basic_block bb;
>> +  FOR_EACH_BB_FN (bb, cfun)
>> +    if (!bitmap_bit_p (in_loop, bb->index))
>> +      {
>> +       /* PHI arguments may contain decl addresses.  */
>> +       for (gphi_iterator gpi = gsi_start_phis (bb); !gsi_end_p (gpi);
>> +            gsi_next (&gpi))
>> +         walk_stmt_load_store_addr_ops (gpi.phi (), escaping, NULL, NULL,
>> +                                        record_memory_decl);
>> +
>> +       for (gimple_stmt_iterator gsi = gsi_start_nondebug_bb (bb);
>> +            !gsi_end_p (gsi); gsi_next_nondebug (&gsi))
>> +         walk_stmt_load_store_addr_ops (gsi_stmt (gsi), escaping,
>> +                                        record_memory_decl, record_memory_decl,
>> +                                        record_memory_decl);
> 
> I'll note this is quadratic in the number of parallelized loops and
> function size.

Yeah, I believe it needs to be. The CFG gets modified as loops get
outlined so cfun requires re-scanning for each loop. I guess it
might be possible to do in linear time as the CFG changes are
purely additive and cannot change things getting new references
outside the loop.

> 
> To me this also looks like optimization, not a safety check that's
> still missing.

I don't think this is an optimization. Leaving these decls in place
means each thread has to allocate stack memory instead of the caller
doing it once and then passing that address. This is just trying to
fix a data race that the pass can accidentally inject.

> What happens if we hoisted/sunk the decl or if we peeled the loop?  You then
> get sharing and thus still miscompilation, no?

Yes, true. It is interesting though that this only happens to surface
with autopar, I guess we got lucky with no other pass doing this?

Maybe the frontend can annotate decls that need to stay private and
passes can respect that. This is what Harald was pointing at in the
last discussion about this:

> So how does OpenMP annotate the temporary (it should be PRIVATE)?
> And can this be used to annotate frontend-generated temporaries
> for the auto-parallelizer?

Until then this is at least trying to fix the issue within this
specific pass (because it is a genuine bug regardless of what
other passes are doing). Though I am a bit unsure now because
there appears to be two directions this is going in:

- Adding a contract to the pass that private variables must be
   annotated as such
- Have the pass conservatively guess which ones are private

May be useful to have both? I don't know.

-- 
Regards,
Dhruv

> 
>> +      }
>> +}
>> +
>>   /* Eliminates the references to local variables from the single entry
>>      single exit region between the ENTRY and EXIT edges.
>>
>> @@ -1288,10 +1345,15 @@ eliminate_local_variables_stmt (edge entry, gimple_stmt_iterator *gsi,
>>      necessary).
>>
>>      2) Dereferencing a local variable -- these are replaced with indirect
>> -   references.  */
>> +   references.
>> +
>> +   A memory decl not in ESCAPING is iteration-private and left untouched, so the
>> +   outliner gives each thread its own copy.  This is a conservative approach to
>> +   avoid hoisting memory that would otherwise end up being overwritten and cause
>> +   data races when written by multiple threads.  */
>>
>>   static void
>> -eliminate_local_variables (edge entry, edge exit)
>> +eliminate_local_variables (edge entry, edge exit, hash_set<tree> *escaping)
>>   {
>>     basic_block bb;
>>     auto_vec<basic_block, 3> body;
>> @@ -1314,7 +1376,8 @@ eliminate_local_variables (edge entry, edge exit)
>>                  has_debug_stmt = true;
>>              }
>>            else
>> -           eliminate_local_variables_stmt (entry, &gsi, &decl_address);
>> +           eliminate_local_variables_stmt (entry, &gsi, &decl_address,
>> +                                           escaping);
>>         }
>>
>>     if (has_debug_stmt)
>> @@ -1322,7 +1385,8 @@ eliminate_local_variables (edge entry, edge exit)
>>         if (bb != entry_bb && bb != exit_bb)
>>          for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>>            if (gimple_debug_bind_p (gsi_stmt (gsi)))
>> -           eliminate_local_variables_stmt (entry, &gsi, &decl_address);
>> +           eliminate_local_variables_stmt (entry, &gsi, &decl_address,
>> +                                           escaping);
>>   }
>>
>>   /* Returns true if expression EXPR is not defined between ENTRY and
>> @@ -2822,6 +2886,11 @@ gen_parallel_loop (class loop *loop,
>>     location_t loc;
>>     gimple *cond_stmt;
>>
>> +  /* Compute this before the loop is duplicated below.  */
>> +  hash_set<tree> escaping;
>> +  if (loop->can_be_parallel)
>> +    gather_escaping_decls (loop, &escaping);
>> +
>>     /* From
>>
>>        ---------------------------------------------------------------------
>> @@ -2994,7 +3063,8 @@ gen_parallel_loop (class loop *loop,
>>        been done for oacc_kernels_p in pass_lower_omp/lower_omp ().  */
>>     if (!oacc_kernels_p)
>>       {
>> -      eliminate_local_variables (entry, exit);
>> +      eliminate_local_variables (entry, exit,
>> +                                loop->can_be_parallel ? &escaping : NULL);
>>         /* In the old loop, move all variables non-local to the loop to a
>>           structure and back, and create separate decls for the variables used in
>>           loop.  */
>> --
>> 2.43.0
>>