Re: [Patch] OpenACC: Fix reduction tree-sharing issue [PR106982]

[Richard Biener <richard.guenther@gmail.com>]

On Mon, Sep 26, 2022 at 11:27 AM Tobias Burnus <tobias@codesourcery.com> wrote:
>
> Hi Richard,
>
> On 26.09.22 10:32, Richard Biener wrote:
>
> On Fri, Sep 23, 2022 at 5:25 PM Tobias Burnus <tobias@codesourcery.com> wrote:
>
> This fixes a tree-sharing ICE. It seems as if all unshare_expr
> I added were required in this case. [...]
>
> looks like v1/v2/v3 are now unshared twice
>
> According to the assert, that's not the case. 'var' is a memory
> reference – and taking out any of the newly added unshare_expr
> will give an ICE with the new *8.c testcase.
>
> better done when its used.  That said, please put the unshares
> at places where new things are built, that's much clearer.  That means
> the 'outgoing' at
>         gimplify_assign (outgoing, teardown_call, &after_join);
>
> The most localized change is the 'else' branch:
>
>   else
> -  v1 = v2 = v3 = var;
> +  {
> +    /* Note that 'var' might be a mem ref.  */
> +    v1 = unshare_expr (var);
> +    v2 = unshare_expr (var);
> +    v3 = unshare_expr (var);
> +    incoming = unshare_expr (incoming);
> +    outgoing = unshare_expr (outgoing);
> +  }
>
> But then I still need to unshare v1/v2/v3 at one other place. Namely:
>
> Either in
>
> - gimplify_assign (v1, setup_call, &before_fork);
> + gimplify_assign (unshare_expr (v1), setup_call, &before_fork);
>
> or in
>    = build_call_expr_internal_loc (loc, IFN_GOACC_REDUCTION,
>    TREE_TYPE (var), 6, init_code,
>    unshare_expr (ref_to_res),
> -  v1, level, op, off);
> +  unshare_expr (v1), level, op, off);
>
>
> Alternatively, I keep the
> else
>  v1 = v2 = v3 = var;
> as is, possible adding the comment there, – and then add the unshare_expr
> for v1/v2/v3/incoming to build_call_expr_internal_loc
> *and* for v1/v2/v3/outgoind to gimplify_assign.
>
> Which variant do you prefer?

I prefer v2a - the unshare_exprs at the sinks where sharing isn't OK.

That variant is OK,

Thanks,
Richard.

> (I have attached both – and the only difference is in omp-low.cc.)
>
> (Certainly, other permutations are possible, one is the one in the first patch,
> but I like either of the two new patches more.)
>
> Tobias
>
> -----------------
> Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955