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

  From: Dhruv Chawla <dhruvc@nvidia.com>

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 <dhruvc@nvidia.com>

	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
  

On Fri, Jul 17, 2026 at 6:47 AM <dhruvc@nvidia.com> wrote:
>
> From: Dhruv Chawla <dhruvc@nvidia.com>
>
> 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 <dhruvc@nvidia.com>
>
>         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))

This looks like you want

    if (VAR_P (base) && auto_var_in_fn_p (base, current_function_decl))

> +    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.

To me this also looks like optimization, not a safety check that's
still missing.
What happens if we hoisted/sunk the decl or if we peeled the loop?  You then
get sharing and thus still miscompilation, no?

> +      }
> +}
> +
>  /* 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
>
  

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 <dhruvc@nvidia.com> wrote:
>>
>> From: Dhruv Chawla <dhruvc@nvidia.com>
>>
>> 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 <dhruvc@nvidia.com>
>>
>>          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.
  

On Fri, Jul 17, 2026 at 2:19 PM Dhruv Chawla <dhruvc@nvidia.com> wrote:
>
> 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 <dhruvc@nvidia.com> wrote:
> >>
> >> From: Dhruv Chawla <dhruvc@nvidia.com>
> >>
> >> 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 <dhruvc@nvidia.com>
> >>
> >>          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.

But I think you could compute, once on the whole function,
a "outermost loop VAR is mentioned".

Also note that for TREE_ADDRESSABLE vars, thus any explicit
ADDR_EXPR you notice (outside of a MEM_REF), means the
variable can be indirectly accessed and you cannot privatize it.

Meaning ...

> >
> > 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.

... you compute the set of variables where it's the implementations choice
to either privatize or share.  IMO for parallelization privatization is always
better since that avoids cache ownership issues and races.  Unless the
variable is only read [in the region], meaning the scheme could be improved
by computing the outermost mention loop but also a set of outer loops
(thus semantically including its children) a variable is not written
to.  For those
sharing should be beneficial over privatization.

> > 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?

What do you mean?  ISTR this is about do concurrent setting can_parallelize
(thus ignoring dependencies)?  No other pass looks at that flag IIRC.

> 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?

Yes, iff the variable is not address taken and we can privatize it (without
copy in/out) then it would help for the frontend to mark such variables.
Together with the IVDEP or CAN_BE_PARALLEL flags this would mean
there are dependences on such marked variables (so the need to
privatize).  If analysis shows we cannot privatize (due to implementation
issues) we can reject parallelization.

> 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

The issue is with can_be_parallel, we either need to refine its
semantics for automatic variables to be suitable for auto-detection
(aka force dependence calculation/verification on a subset of refs)
or require the frontends to constrain the set of variables affected
(by force-privatizing for example).  But then the set of variables covered
need to be more clearly defined since optimizations can bring in ones
from outside, so there would need to be sth like a BLOCK associated
with a (can_be_parallel) loop referencing variables covered by the
contract.  I think OMP gets away with such issues because it does
lowering quite early.  We could, for example, make the contract that
can_be_parallel loops have to reference the loop BLOCK on the
loop exit condition.

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

The first step should be to design the contract between autopar
and can_be_parallel marked loops, in a way that autopar
can actually validate.

Richard.

>
> --
> 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
> >>