On Fri, 8 Nov 2024, Jakub Jelinek wrote:
> Hi!
>
> My https://gcc.gnu.org/pipermail/gcc-patches/2024-November/668065.html
> patch regressed
> +FAIL: g++.dg/tm/pr45940-3.C -std=gnu++11 (internal compiler error: in
> create_tmp_var, at gimple-expr.cc:484)
> +FAIL: g++.dg/tm/pr45940-3.C -std=gnu++11 (test for excess errors)
> +FAIL: g++.dg/tm/pr45940-3.C -std=gnu++14 (internal compiler error: in
> create_tmp_var, at gimple-expr.cc:484)
> +FAIL: g++.dg/tm/pr45940-3.C -std=gnu++14 (test for excess errors)
> ...
> +FAIL: g++.dg/tm/pr45940-4.C -std=gnu++26 (internal compiler error: in
> create_tmp_var, at gimple-expr.cc:484)
> +FAIL: g++.dg/tm/pr45940-4.C -std=gnu++26 (test for excess errors)
> +FAIL: g++.dg/tm/pr45940-4.C -std=gnu++98 (internal compiler error: in
> create_tmp_var, at gimple-expr.cc:484)
> +FAIL: g++.dg/tm/pr45940-4.C -std=gnu++98 (test for excess errors)
> tests, but it turns out it is a preexisting bug.
> If I modify the pr45940-3.C testcase
> --- gcc/testsuite/g++.dg/tm/pr45940-3.C 2020-01-12 11:54:37.258400660
> +0100
> +++ gcc/testsuite/g++.dg/tm/pr45940-3.C 2024-11-08 10:35:11.918390743
> +0100
> @@ -16,6 +16,7 @@ class sp_counted_base
> {
> protected:
> int use_count_; // #shared
> + int a, b, c, d, e, f, g, h, i, j, k, l, m, n, o, p, q, r, s, t, u, v, w,
> x, y, z, aa, ab, ac, ad, ae, af;
> public:
> __attribute__((transaction_safe))
> virtual void dispose() = 0; // nothrow
> then it ICEs already on vanilla trunk.
>
> The problem is that expand_assign_tm just wants to force it into
> TM memcpy argument, if is_gimple_reg (reg), then it creates a temporary,
> stores the value there and takes temporary address, otherwise it takes
> address of rhs. That doesn't work if rhs is an empty CONSTRUCTOR with
> C++ non-POD type (TREE_ADDRESSABLE type), we ICE trying to create temporary,
> because we shouldn't be creating a temporary.
> Now before my patch with the CONSTRUCTOR only having a vtable pointer
> (64bit) and 32-bit field, we gimplified the zero initialization just
> as storing of 0s to the 2 fields, but as we are supposed to also clear
> padding bits, we now gimplify it as MEM[...] = {}; to make sure
> even the padding bits are cleared. With the adjusted testcase,
> we gimplified it even before as MEM[...] = {}; because it was simply
> too large and clearing everything looked beneficial.
>
> The following patch fixes this ICE by using TM memset, it is both
> wasteful to force zero constructor into a temporary just to TM memcpy
> it into the lhs, and in C++ cases like this invalid.
>
> Ok for trunk if it passes bootstrap/regtest?
OK.
Richard.
> 2024-11-08 Jakub Jelinek <ja...@redhat.com>
>
> * trans-mem.cc (expand_assign_tm): Don't take address
> of empty CONSTRUCTOR, instead use BUILT_IN_TM_MEMSET
> to clear lhs in that case. Formatting fixes.
>
> --- gcc/trans-mem.cc.jj 2024-10-25 10:00:29.527767013 +0200
> +++ gcc/trans-mem.cc 2024-11-08 09:55:08.963557301 +0100
> @@ -2442,26 +2442,33 @@ expand_assign_tm (struct tm_region *regi
> gcall = gimple_build_assign (rtmp, rhs);
> gsi_insert_before (gsi, gcall, GSI_SAME_STMT);
> }
> + else if (TREE_CODE (rhs) == CONSTRUCTOR
> + && CONSTRUCTOR_NELTS (rhs) == 0)
> + {
> + /* Don't take address of an empty CONSTRUCTOR, it might not
> + work for C++ non-POD constructors at all and otherwise
> + would be inefficient. Use tm memset to clear lhs. */
> + gcc_assert (!load_p && store_p);
> + rhs_addr = integer_zero_node;
> + }
> else
> rhs_addr = gimplify_addr (gsi, rhs);
>
> // Choose the appropriate memory transfer function.
> - if (load_p && store_p)
> - {
> - // ??? Figure out if there's any possible overlap between
> - // the LHS and the RHS and if not, use MEMCPY.
> - copy_fn = builtin_decl_explicit (BUILT_IN_TM_MEMMOVE);
> - }
> + if (store_p
> + && TREE_CODE (rhs) == CONSTRUCTOR
> + && CONSTRUCTOR_NELTS (rhs) == 0)
> + copy_fn = builtin_decl_explicit (BUILT_IN_TM_MEMSET);
> + else if (load_p && store_p)
> + // ??? Figure out if there's any possible overlap between
> + // the LHS and the RHS and if not, use MEMCPY.
> + copy_fn = builtin_decl_explicit (BUILT_IN_TM_MEMMOVE);
> else if (load_p)
> - {
> - // Note that the store is non-transactional and cannot overlap.
> - copy_fn = builtin_decl_explicit (BUILT_IN_TM_MEMCPY_RTWN);
> - }
> + // Note that the store is non-transactional and cannot overlap.
> + copy_fn = builtin_decl_explicit (BUILT_IN_TM_MEMCPY_RTWN);
> else
> - {
> - // Note that the load is non-transactional and cannot overlap.
> - copy_fn = builtin_decl_explicit (BUILT_IN_TM_MEMCPY_RNWT);
> - }
> + // Note that the load is non-transactional and cannot overlap.
> + copy_fn = builtin_decl_explicit (BUILT_IN_TM_MEMCPY_RNWT);
>
> gcall = gimple_build_call (copy_fn, 3, lhs_addr, rhs_addr,
> TYPE_SIZE_UNIT (TREE_TYPE (lhs)));
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
