[Bug middle-end/55882] [4.7/4.8 Regression] unaligned load/store : incorrect struct offset

["rguenth at gcc dot gnu.org" <gcc-bugzilla at gcc dot gnu dot org>]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55882

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
             Status|WAITING                     |NEW
                 CC|                            |ebotcazou at gcc dot
                   |                            |gnu.org, rguenth at gcc dot
                   |                            |gnu.org
          Component|c                           |middle-end
   Target Milestone|---                         |4.7.3
            Summary|unaligned load/store :      |[4.7/4.8 Regression]
                   |incorrect struct offset     |unaligned load/store :
                   |                            |incorrect struct offset

--- Comment #10 from Richard Biener <rguenth at gcc dot gnu.org> 2013-01-09 10:41:28 UTC ---
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 ...