| Message ID | Zy3ei9uBA1fPNjVr@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | trans-mem: Fix ICE caused by expand_assign_tm | expand |
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 <jakub@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 > >
--- 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? 2024-11-08 Jakub Jelinek <jakub@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)));