On Mon, Sep 26, 2022 at 11:27 AM Tobias Burnus <tob...@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 <tob...@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
