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