On Tue, 10 May 2016, Jakub Jelinek wrote: > On Tue, May 10, 2016 at 10:18:59AM +0200, Richard Biener wrote: > > There are two options - not apply this folding if it doesn't preserve > > TBAA behavior (first patch) or preserve TBAA behavior by wrapping > > the base into a MEM_REF (which also requires fixing > > reference_alias_ptr_type, sth which we need to do anyway). > > See the two patches below. > > > > Incidentially the folding (which I still consider premature) prevents > > CSE on the GIMPLE level as well: > > > > <bb 2>: > > MEM[(struct &)this_2(D)] ={v} {CLOBBER}; > > _4 = &this_2(D)->m_str; > > MEM[(struct &)this_2(D)] ={v} {CLOBBER}; > > MEM[(struct short_t &)this_2(D)].h.is_short = 1; > > MEM[(struct short_t &)this_2(D)].h.length = 0; > > MEM[(struct short_t &)this_2(D)].data[0] = 0; > > _19 = BIT_FIELD_REF <MEM[(void *)this_2(D)], 8, 0>; > > _20 = _19 & 1; > > if (_20 != 0) > > > > Here we can't CSE the is_short access and optimize the compare. > > So on trunk I am thinking of at least removing the compare > > against constant case. [I've removed the whole code twice in > > GCC history and it always got installed back ...] > > > > Bootstrap and regtest of both patches running on x86_64-unknown-linux-gnu. > > > > Any preference for GCC 6? > > I think I prefer the second patch for the branch.
On trunk this exposed an issue with the gimplify-into-SSA patch and I catched an issue with passing the original ref. Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk sofar. Richard. 2016-05-11 Richard Biener <rguent...@suse.de> PR middle-end/71002 * alias.c (reference_alias_ptr_type): Preserve alias-set zero if the langhook insists on it. * fold-const.c (make_bit_field_ref): Add arg for the original reference and preserve its alias-set. (decode_field_reference): Take exp by reference and adjust it to the original memory reference. (optimize_bit_field_compare): Adjust callers. (fold_truth_andor_1): Likewise. * gimplify.c (gimplify_expr): Adjust in-SSA form test. * g++.dg/torture/pr71002.C: New testcase. Index: gcc/alias.c =================================================================== *** gcc/alias.c (revision 236032) --- gcc/alias.c (working copy) *************** reference_alias_ptr_type_1 (tree *t) *** 769,774 **** --- 769,778 ---- tree reference_alias_ptr_type (tree t) { + /* If the frontend assigns this alias-set zero, preserve that. */ + if (lang_hooks.get_alias_set (t) == 0) + return ptr_type_node; + tree ptype = reference_alias_ptr_type_1 (&t); /* If there is a given pointer type for aliasing purposes, return it. */ if (ptype != NULL_TREE) Index: gcc/fold-const.c =================================================================== *** gcc/fold-const.c (revision 236069) --- gcc/fold-const.c (working copy) *************** static enum tree_code compcode_to_compar *** 117,130 **** static int operand_equal_for_comparison_p (tree, tree, tree); static int twoval_comparison_p (tree, tree *, tree *, int *); static tree eval_subst (location_t, tree, tree, tree, tree, tree); - static tree make_bit_field_ref (location_t, tree, tree, - HOST_WIDE_INT, HOST_WIDE_INT, int, int); static tree optimize_bit_field_compare (location_t, enum tree_code, tree, tree, tree); - static tree decode_field_reference (location_t, tree, HOST_WIDE_INT *, - HOST_WIDE_INT *, - machine_mode *, int *, int *, int *, - tree *, tree *); static int simple_operand_p (const_tree); static bool simple_operand_p_2 (tree); static tree range_binop (enum tree_code, tree, tree, int, tree, int); --- 117,124 ---- *************** distribute_real_division (location_t loc *** 3803,3817 **** /* Return a BIT_FIELD_REF of type TYPE to refer to BITSIZE bits of INNER starting at BITPOS. The field is unsigned if UNSIGNEDP is nonzero ! and uses reverse storage order if REVERSEP is nonzero. */ static tree ! make_bit_field_ref (location_t loc, tree inner, tree type, HOST_WIDE_INT bitsize, HOST_WIDE_INT bitpos, int unsignedp, int reversep) { tree result, bftype; if (bitpos == 0 && !reversep) { tree size = TYPE_SIZE (TREE_TYPE (inner)); --- 3797,3819 ---- /* Return a BIT_FIELD_REF of type TYPE to refer to BITSIZE bits of INNER starting at BITPOS. The field is unsigned if UNSIGNEDP is nonzero ! and uses reverse storage order if REVERSEP is nonzero. ORIG_INNER ! is the original memory reference used to preserve the alias set of ! the access. */ static tree ! make_bit_field_ref (location_t loc, tree inner, tree orig_inner, tree type, HOST_WIDE_INT bitsize, HOST_WIDE_INT bitpos, int unsignedp, int reversep) { tree result, bftype; + if (get_alias_set (inner) != get_alias_set (orig_inner)) + inner = fold_build2 (MEM_REF, TREE_TYPE (inner), + build_fold_addr_expr (inner), + build_int_cst + (reference_alias_ptr_type (orig_inner), 0)); + if (bitpos == 0 && !reversep) { tree size = TYPE_SIZE (TREE_TYPE (inner)); *************** optimize_bit_field_compare (location_t l *** 3937,3949 **** and return. */ return fold_build2_loc (loc, code, compare_type, fold_build2_loc (loc, BIT_AND_EXPR, unsigned_type, ! make_bit_field_ref (loc, linner, unsigned_type, nbitsize, nbitpos, 1, lreversep), mask), fold_build2_loc (loc, BIT_AND_EXPR, unsigned_type, ! make_bit_field_ref (loc, rinner, unsigned_type, nbitsize, nbitpos, 1, rreversep), --- 3939,3951 ---- and return. */ return fold_build2_loc (loc, code, compare_type, fold_build2_loc (loc, BIT_AND_EXPR, unsigned_type, ! make_bit_field_ref (loc, linner, lhs, unsigned_type, nbitsize, nbitpos, 1, lreversep), mask), fold_build2_loc (loc, BIT_AND_EXPR, unsigned_type, ! make_bit_field_ref (loc, rinner, rhs, unsigned_type, nbitsize, nbitpos, 1, rreversep), *************** optimize_bit_field_compare (location_t l *** 3988,3995 **** /* Make a new bitfield reference, shift the constant over the appropriate number of bits and mask it with the computed mask (in case this was a signed field). If we changed it, make a new one. */ ! lhs = make_bit_field_ref (loc, linner, unsigned_type, nbitsize, nbitpos, 1, ! lreversep); rhs = const_binop (BIT_AND_EXPR, const_binop (LSHIFT_EXPR, --- 3990,3997 ---- /* Make a new bitfield reference, shift the constant over the appropriate number of bits and mask it with the computed mask (in case this was a signed field). If we changed it, make a new one. */ ! lhs = make_bit_field_ref (loc, linner, lhs, unsigned_type, ! nbitsize, nbitpos, 1, lreversep); rhs = const_binop (BIT_AND_EXPR, const_binop (LSHIFT_EXPR, *************** optimize_bit_field_compare (location_t l *** 4028,4038 **** do anything with. */ static tree ! decode_field_reference (location_t loc, tree exp, HOST_WIDE_INT *pbitsize, HOST_WIDE_INT *pbitpos, machine_mode *pmode, int *punsignedp, int *preversep, int *pvolatilep, tree *pmask, tree *pand_mask) { tree outer_type = 0; tree and_mask = 0; tree mask, inner, offset; --- 4030,4041 ---- do anything with. */ static tree ! decode_field_reference (location_t loc, tree *exp_, HOST_WIDE_INT *pbitsize, HOST_WIDE_INT *pbitpos, machine_mode *pmode, int *punsignedp, int *preversep, int *pvolatilep, tree *pmask, tree *pand_mask) { + tree exp = *exp_; tree outer_type = 0; tree and_mask = 0; tree mask, inner, offset; *************** decode_field_reference (location_t loc, *** 4069,4074 **** --- 4072,4079 ---- || TREE_CODE (inner) == PLACEHOLDER_EXPR) return 0; + *exp_ = exp; + /* If the number of bits in the reference is the same as the bitsize of the outer type, then the outer type gives the signedness. Otherwise (in case of a small bitfield) the signedness is unchanged. */ *************** fold_truth_andor_1 (location_t loc, enum *** 5677,5695 **** ll_reversep = lr_reversep = rl_reversep = rr_reversep = 0; volatilep = 0; ! ll_inner = decode_field_reference (loc, ll_arg, &ll_bitsize, &ll_bitpos, &ll_mode, &ll_unsignedp, &ll_reversep, &volatilep, &ll_mask, &ll_and_mask); ! lr_inner = decode_field_reference (loc, lr_arg, &lr_bitsize, &lr_bitpos, &lr_mode, &lr_unsignedp, &lr_reversep, &volatilep, &lr_mask, &lr_and_mask); ! rl_inner = decode_field_reference (loc, rl_arg, &rl_bitsize, &rl_bitpos, &rl_mode, &rl_unsignedp, &rl_reversep, &volatilep, &rl_mask, &rl_and_mask); ! rr_inner = decode_field_reference (loc, rr_arg, &rr_bitsize, &rr_bitpos, &rr_mode, &rr_unsignedp, &rr_reversep, &volatilep, &rr_mask, &rr_and_mask); --- 5682,5700 ---- ll_reversep = lr_reversep = rl_reversep = rr_reversep = 0; volatilep = 0; ! ll_inner = decode_field_reference (loc, &ll_arg, &ll_bitsize, &ll_bitpos, &ll_mode, &ll_unsignedp, &ll_reversep, &volatilep, &ll_mask, &ll_and_mask); ! lr_inner = decode_field_reference (loc, &lr_arg, &lr_bitsize, &lr_bitpos, &lr_mode, &lr_unsignedp, &lr_reversep, &volatilep, &lr_mask, &lr_and_mask); ! rl_inner = decode_field_reference (loc, &rl_arg, &rl_bitsize, &rl_bitpos, &rl_mode, &rl_unsignedp, &rl_reversep, &volatilep, &rl_mask, &rl_and_mask); ! rr_inner = decode_field_reference (loc, &rr_arg, &rr_bitsize, &rr_bitpos, &rr_mode, &rr_unsignedp, &rr_reversep, &volatilep, &rr_mask, &rr_and_mask); *************** fold_truth_andor_1 (location_t loc, enum *** 5851,5862 **** lr_mask = const_binop (BIT_IOR_EXPR, lr_mask, rr_mask); if (lnbitsize == rnbitsize && xll_bitpos == xlr_bitpos) { ! lhs = make_bit_field_ref (loc, ll_inner, lntype, lnbitsize, lnbitpos, ll_unsignedp || rl_unsignedp, ll_reversep); if (! all_ones_mask_p (ll_mask, lnbitsize)) lhs = build2 (BIT_AND_EXPR, lntype, lhs, ll_mask); ! rhs = make_bit_field_ref (loc, lr_inner, rntype, rnbitsize, rnbitpos, lr_unsignedp || rr_unsignedp, lr_reversep); if (! all_ones_mask_p (lr_mask, rnbitsize)) rhs = build2 (BIT_AND_EXPR, rntype, rhs, lr_mask); --- 5856,5869 ---- lr_mask = const_binop (BIT_IOR_EXPR, lr_mask, rr_mask); if (lnbitsize == rnbitsize && xll_bitpos == xlr_bitpos) { ! lhs = make_bit_field_ref (loc, ll_inner, ll_arg, ! lntype, lnbitsize, lnbitpos, ll_unsignedp || rl_unsignedp, ll_reversep); if (! all_ones_mask_p (ll_mask, lnbitsize)) lhs = build2 (BIT_AND_EXPR, lntype, lhs, ll_mask); ! rhs = make_bit_field_ref (loc, lr_inner, lr_arg, ! rntype, rnbitsize, rnbitpos, lr_unsignedp || rr_unsignedp, lr_reversep); if (! all_ones_mask_p (lr_mask, rnbitsize)) rhs = build2 (BIT_AND_EXPR, rntype, rhs, lr_mask); *************** fold_truth_andor_1 (location_t loc, enum *** 5878,5888 **** { tree type; ! lhs = make_bit_field_ref (loc, ll_inner, lntype, ll_bitsize + rl_bitsize, MIN (ll_bitpos, rl_bitpos), ll_unsignedp, ll_reversep); ! rhs = make_bit_field_ref (loc, lr_inner, rntype, lr_bitsize + rr_bitsize, MIN (lr_bitpos, rr_bitpos), lr_unsignedp, lr_reversep); --- 5885,5895 ---- { tree type; ! lhs = make_bit_field_ref (loc, ll_inner, ll_arg, lntype, ll_bitsize + rl_bitsize, MIN (ll_bitpos, rl_bitpos), ll_unsignedp, ll_reversep); ! rhs = make_bit_field_ref (loc, lr_inner, lr_arg, rntype, lr_bitsize + rr_bitsize, MIN (lr_bitpos, rr_bitpos), lr_unsignedp, lr_reversep); *************** fold_truth_andor_1 (location_t loc, enum *** 5947,5953 **** reference we will make. Unless the mask is all ones the width of that field, perform the mask operation. Then compare with the merged constant. */ ! result = make_bit_field_ref (loc, ll_inner, lntype, lnbitsize, lnbitpos, ll_unsignedp || rl_unsignedp, ll_reversep); ll_mask = const_binop (BIT_IOR_EXPR, ll_mask, rl_mask); --- 5954,5961 ---- reference we will make. Unless the mask is all ones the width of that field, perform the mask operation. Then compare with the merged constant. */ ! result = make_bit_field_ref (loc, ll_inner, ll_arg, ! lntype, lnbitsize, lnbitpos, ll_unsignedp || rl_unsignedp, ll_reversep); ll_mask = const_binop (BIT_IOR_EXPR, ll_mask, rl_mask); Index: gcc/gimplify.c =================================================================== *** gcc/gimplify.c (revision 236077) --- gcc/gimplify.c (working copy) *************** gimplify_expr (tree *expr_p, gimple_seq *** 10452,10458 **** in suitable form. Re-gimplifying would mark the address operand addressable. Always gimplify when not in SSA form as we still may have to gimplify decls with value-exprs. */ ! if (!gimplify_ctxp || !gimplify_ctxp->into_ssa || !is_gimple_mem_ref_addr (TREE_OPERAND (*expr_p, 0))) { ret = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, post_p, --- 10452,10458 ---- in suitable form. Re-gimplifying would mark the address operand addressable. Always gimplify when not in SSA form as we still may have to gimplify decls with value-exprs. */ ! if (!gimplify_ctxp || !gimple_in_ssa_p (cfun) || !is_gimple_mem_ref_addr (TREE_OPERAND (*expr_p, 0))) { ret = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, post_p, Index: gcc/testsuite/g++.dg/torture/pr71002.C =================================================================== *** gcc/testsuite/g++.dg/torture/pr71002.C (revision 0) --- gcc/testsuite/g++.dg/torture/pr71002.C (revision 0) *************** *** 0 **** --- 1,160 ---- + // { dg-do run } + + using size_t = __SIZE_TYPE__; + + inline void* operator new(size_t, void* p) noexcept + { return p; } + + inline void operator delete(void*, void*) + { } + + struct long_t + { + size_t is_short : 1; + size_t length : (__SIZEOF_SIZE_T__ * __CHAR_BIT__ - 1); + size_t capacity; + char* pointer; + }; + + union long_raw_t { + unsigned char data[sizeof(long_t)]; + struct __attribute__((aligned(alignof(long_t)))) { } align; + }; + + struct short_header + { + unsigned char is_short : 1; + unsigned char length : (__CHAR_BIT__ - 1); + }; + + struct short_t + { + short_header h; + char data[23]; + }; + + union repr_t + { + long_raw_t r; + short_t s; + + const short_t& short_repr() const + { return s; } + + const long_t& long_repr() const + { return *static_cast<const long_t*>(static_cast<const void*>(&r)); } + + short_t& short_repr() + { return s; } + + long_t& long_repr() + { return *static_cast<long_t*>(static_cast<void*>(&r)); } + }; + + class string + { + public: + string() + { + short_t& s = m_repr.short_repr(); + s.h.is_short = 1; + s.h.length = 0; + s.data[0] = '\0'; + } + + string(const char* str) + { + size_t length = __builtin_strlen(str); + if (length + 1 > 23) { + long_t& l = m_repr.long_repr(); + l.is_short = 0; + l.length = length; + l.capacity = length + 1; + l.pointer = new char[l.capacity]; + __builtin_memcpy(l.pointer, str, length + 1); + } else { + short_t& s = m_repr.short_repr(); + s.h.is_short = 1; + s.h.length = length; + __builtin_memcpy(s.data, str, length + 1); + } + } + + string(string&& other) + : string{} + { + swap_data(other); + } + + ~string() + { + if (!is_short()) { + delete[] m_repr.long_repr().pointer; + } + } + + size_t length() const + { return is_short() ? short_length() : long_length(); } + + private: + bool is_short() const + { return m_repr.s.h.is_short != 0; } + + size_t short_length() const + { return m_repr.short_repr().h.length; } + + size_t long_length() const + { return m_repr.long_repr().length; } + + void swap_data(string& other) + { + if (is_short()) { + if (other.is_short()) { + repr_t tmp(m_repr); + m_repr = other.m_repr; + other.m_repr = tmp; + } else { + short_t short_backup(m_repr.short_repr()); + m_repr.short_repr().~short_t(); + ::new(&m_repr.long_repr()) long_t(other.m_repr.long_repr()); + other.m_repr.long_repr().~long_t(); + ::new(&other.m_repr.short_repr()) short_t(short_backup); + } + } else { + if (other.is_short()) { + short_t short_backup(other.m_repr.short_repr()); + other.m_repr.short_repr().~short_t(); + ::new(&other.m_repr.long_repr()) long_t(m_repr.long_repr()); + m_repr.long_repr().~long_t(); + ::new(&m_repr.short_repr()) short_t(short_backup); + } else { + long_t tmp(m_repr.long_repr()); + m_repr.long_repr() = other.m_repr.long_repr(); + other.m_repr.long_repr() = tmp; + } + } + } + + repr_t m_repr; + }; + + struct foo + { + __attribute__((noinline)) + foo(string str) + : m_str{static_cast<string&&>(str)}, + m_len{m_str.length()} + { } + + string m_str; + size_t m_len; + }; + + int main() + { + foo f{"the quick brown fox jumps over the lazy dog"}; + if (f.m_len == 0) { + __builtin_abort(); + } + return 0; + }