From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5727 invoked by alias); 9 Jan 2013 11:36:07 -0000 Received: (qmail 3447 invoked by uid 48); 9 Jan 2013 11:35:35 -0000 From: "rguenth at gcc dot gnu.org" To: gcc-bugs@gcc.gnu.org Subject: [Bug middle-end/55882] [4.7/4.8 Regression] unaligned load/store : incorrect struct offset Date: Wed, 09 Jan 2013 11:36:00 -0000 X-Bugzilla-Reason: CC X-Bugzilla-Type: changed X-Bugzilla-Watch-Reason: None X-Bugzilla-Product: gcc X-Bugzilla-Component: middle-end X-Bugzilla-Keywords: wrong-code X-Bugzilla-Severity: normal X-Bugzilla-Who: rguenth at gcc dot gnu.org X-Bugzilla-Status: NEW X-Bugzilla-Priority: P3 X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org X-Bugzilla-Target-Milestone: 4.7.3 X-Bugzilla-Changed-Fields: Message-ID: In-Reply-To: References: X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/ Auto-Submitted: auto-generated Content-Type: text/plain; charset="UTF-8" MIME-Version: 1.0 Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-bugs-owner@gcc.gnu.org X-SW-Source: 2013-01/txt/msg00769.txt.bz2 http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55882 --- Comment #12 from Richard Biener 2013-01-09 11:35:33 UTC --- (In reply to comment #10) > Eric, I am double-checking my patch and I believe all this 'bitpos' handling > in set_mem_attributes_minus_bitpos (and/or MEM_OFFSET in general) looks > suspicious. > > /* For a MEM rtx, the offset from the start of MEM_EXPR. */ > #define MEM_OFFSET(RTX) (get_mem_attrs (RTX)->offset) > > I read from this that the XEXP (RTX, 0) points to MEM_EXPR + MEM_OFFSET > (if MEM_OFFSET_KNOWN_P, and if !MEM_OFFSET_KNOWN_P we don't know how > XEXP (RTX, 0) and MEM_EXPR relate). > > Now, in expand_assignment we do > > tem = get_inner_reference (to, &bitsize, &bitpos, &offset, &mode1, > &unsignedp, &volatilep, true); > > if (TREE_CODE (to) == COMPONENT_REF > && DECL_BIT_FIELD_TYPE (TREE_OPERAND (to, 1))) > get_bit_range (&bitregion_start, &bitregion_end, to, &bitpos, &offset); > > ... > to_rtx = expand_expr (tem, NULL_RTX, VOIDmode, EXPAND_WRITE); > ... > offset it with variable parts from 'offset' > ... > set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos); > > but bitpos here is _not_ ontop of 'to' but extracted from 'to' (and > eventually modified by get_bit_range which may shift things to 'offset'). > > The only 'bitpos' that should be passed to set_mem_attributes_minus_bitpos > is one that reflects - in the bitfield access case - that we actually > access things at a different position. But at this point we don't know > what optimize_bitfield_assignment_op or store_field will eventually do. > Also MEM_OFFSET is in bytes while I believe 'bitpos' can end up as > an actual bit position, so > > /* If we modified OFFSET based on T, then subtract the outstanding > bit position offset. Similarly, increase the size of the accessed > object to contain the negative offset. */ > if (apply_bitpos) > { > gcc_assert (attrs.offset_known_p); > attrs.offset -= apply_bitpos / BITS_PER_UNIT; > if (attrs.size_known_p) > attrs.size += apply_bitpos / BITS_PER_UNIT; > } > > (whatever this comment means). I believe my fix is correct following > the MEM_OFFSET description and guessing at what the argument to > set_mem_attributes_minus_bitpos means by looking at its use. But I > believe that expand_assignment should pass zero as bitpos to > set_mem_attributes_minus_bitpos (making the only caller that calls this > function with bitpos != 0 go). > > In this testcase we want to access sth at offset 8 bytes, 0 bits but > the memory model tells us the bitregion to consider is > everything from offset 6 to offset 14 so instead of the correct > initial mem > > (mem/j:HI (plus:SI (reg/f:SI 206) > (const_int 8 [0x6])) [0 dmfe[i_1].use_alt_rd_dqs S2 A32]) > > (with 4 byte alignemnt!) we create > > (mem/j:BLK (plus:SI (reg/f:SI 206) > (const_int 6 [0x6])) [0 dmfe[i_1].use_alt_rd_dqs+-6 S8 A32]) > > where the alignment is bogus. > > Thus, given the above general MEM_OFFSET analysis I'd say the following > (ontop of my previous patch) should fix things more correctly: > > Index: gcc/expr.c > =================================================================== > --- gcc/expr.c (revision 195014) > +++ gcc/expr.c (working copy) > @@ -4669,7 +4669,7 @@ expand_assignment (tree to, tree from, b > || TREE_CODE (TREE_TYPE (to)) == ARRAY_TYPE) > { > enum machine_mode mode1; > - HOST_WIDE_INT bitsize, bitpos; > + HOST_WIDE_INT bitsize, bitpos, bitpos_adj; > unsigned HOST_WIDE_INT bitregion_start = 0; > unsigned HOST_WIDE_INT bitregion_end = 0; > tree offset; > @@ -4683,9 +4683,15 @@ expand_assignment (tree to, tree from, b > tem = get_inner_reference (to, &bitsize, &bitpos, &offset, &mode1, > &unsignedp, &volatilep, true); > > + bitpos_adj = 0; > if (TREE_CODE (to) == COMPONENT_REF > && DECL_BIT_FIELD_TYPE (TREE_OPERAND (to, 1))) > - get_bit_range (&bitregion_start, &bitregion_end, to, &bitpos, &offset); > + { > + HOST_WIDE_INT orig_bitpos = bitpos; > + get_bit_range (&bitregion_start, &bitregion_end, > + to, &bitpos, &offset); > + bitpos_adj = orig_bitpos - bitpos; > + } > > /* If we are going to use store_bit_field and extract_bit_field, > make sure to_rtx will be safe for multiple use. */ > @@ -4839,7 +4845,7 @@ expand_assignment (tree to, tree from, b > DECL_RTX of the parent struct. Don't munge it. */ > to_rtx = shallow_copy_rtx (to_rtx); > > - set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos); > + set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos_adj); > > /* Deal with volatile and readonly fields. The former is only > done for MEM. Also set MEM_KEEP_ALIAS_SET_P if needed. */ > > that is, we always pass zero to set_mem_attributes_minus_bitpos unless > the original offset was modified. > > Hmm, but we really pass in a MEM that only covers tem + offset and not > bitpos. But MEM_EXPR does not reflect that - we pass in the original > 'to', not sth like *(&tem + offset). Maybe in very distant times > all the stripping of component refs from MEM_EXPR compensated for that? > > What a mess ... Forget all that - it seems to be correct. With the fix from comment #11. Apart from the bit-align issue.