[Bug c/55882] New: unaligned load/store : incorrect struct offset

[Bug c/55882] New: unaligned load/store : incorrect struct offset

[jan.smets@alcatel-lucent.com]

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

             Bug #: 55882
           Summary: unaligned load/store : incorrect struct offset
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: jan.smets@alcatel-lucent.com


void testfunction(somestruct * chip)
{
    unsigned i;
    for (i = 0; i < 4; i++)
    {
       {
tUint64 mask = ((((((((((chip->pp_mask) & (0x00200000)) ? (0x08) :
(((chip->pp_mask) & (0x00000200)) ? (0x0C) : 0xDEADBEEF))))-1) - (0) +
1)==64)?0ull:((1ull)<<((((((((((chip->pp_mask) & (0x00200000)) ? (0x08) :
(((chip->pp_mask) & (0x00000200)) ? (0x0C) : 0xDEADBEEF))))-1) - (0) + 1) <
(63)) ? (((((((chip->pp_mask) & (0x00200000)) ? (0x08) : (((chip->pp_mask) &
(0x00000200)) ? (0x0C) : 0xDEADBEEF))))-1) - (0) + 1) : (63))))) - 1) << ((1) ?
(0) : ((((((chip->pp_mask) & (0x00200000)) ? (0x08) : (((chip->pp_mask) &
(0x00000200)) ? (0x0C) : 0xDEADBEEF)))) - 1 - ((((((chip->pp_mask) &
(0x00200000)) ? (0x08) : (((chip->pp_mask) & (0x00000200)) ? (0x0C) :
0xDEADBEEF))))-1)));
    chip->config->smth[i].xyz = chip->config->smth[i].xyz & ~mask;
};
    }

The structure 'somestruct' is hard to reduce and for various reasons I can not
disclose it. 

GCC 4.6 :
=========
00000000 <testfunction>:
   0:   27bdffe8    addiu   sp,sp,-24
   4:   afbe0014    sw  s8,20(sp)
   8:   03a0f02d    move    s8,sp
   c:   afc40018    sw  a0,24(s8)
  10:   afc00000    sw  zero,0(s8)
  14:   08000059    j   164 <testfunction+0x164>
  18:   00000000    nop
  1c:   8fc20018    lw  v0,24(s8)
  20:   8c431ab8    lw  v1,6840(v0)
  24:   3c020020    lui v0,0x20
  28:   00621024    and v0,v1,v0
  2c:   14400006    bnez    v0,48 <testfunction+0x48>
  30:   00000000    nop
  34:   8fc20018    lw  v0,24(s8)
  38:   8c421ab8    lw  v0,6840(v0)
  3c:   30420200    andi    v0,v0,0x200
  40:   10400018    beqz    v0,a4 <testfunction+0xa4>
  44:   00000000    nop
  48:   8fc20018    lw  v0,24(s8)
  4c:   8c431ab8    lw  v1,6840(v0)
  50:   3c020020    lui v0,0x20
  54:   00621024    and v0,v1,v0
  58:   1440000e    bnez    v0,94 <testfunction+0x94>
  5c:   00000000    nop
  60:   8fc20018    lw  v0,24(s8)
  64:   8c421ab8    lw  v0,6840(v0)
  68:   30420200    andi    v0,v0,0x200
  6c:   10400005    beqz    v0,84 <testfunction+0x84>
  70:   00000000    nop
  74:   24030fff    li  v1,4095
  78:   0000102d    move    v0,zero
  7c:   08000023    j   8c <testfunction+0x8c>
  80:   00000000    nop
  84:   2403ffff    li  v1,-1
  88:   2402ffff    li  v0,-1
  8c:   08000027    j   9c <testfunction+0x9c>
  90:   00000000    nop
  94:   240300ff    li  v1,255
  98:   0000102d    move    v0,zero
  9c:   0800002c    j   b0 <testfunction+0xb0>
  a0:   00000000    nop
  a4:   2403ffff    li  v1,-1
  a8:   3c027fff    lui v0,0x7fff
  ac:   3442ffff    ori v0,v0,0xffff
  b0:   afc3000c    sw  v1,12(s8)
  b4:   afc20008    sw  v0,8(s8)
  b8:   8fc20018    lw  v0,24(s8)
  bc:   8c461ac0    lw  a2,6848(v0)
  c0:   8fc20018    lw  v0,24(s8)
  c4:   8c451ac0    lw  a1,6848(v0)
  c8:   8fc40000    lw  a0,0(s8)
  cc:   0080182d    move    v1,a0
  d0:   00031080    sll v0,v1,0x2
  d4:   0040182d    move    v1,v0
  d8:   00031080    sll v0,v1,0x2
  dc:   00431023    subu    v0,v0,v1
  e0:   00441023    subu    v0,v0,a0
  e4:   000211c0    sll v0,v0,0x7
  e8:   00a21021    addu    v0,a1,v0
  ec:   8c4212d0    lw  v0,4816(v0)
  f0:   00021502    srl v0,v0,0x14
  f4:   3042ffff    andi    v0,v0,0xffff
  f8:   0040182d    move    v1,v0
  fc:   97c2000e    lhu v0,14(s8)
 100:   00021027    nor v0,zero,v0
 104:   3042ffff    andi    v0,v0,0xffff
 108:   00621024    and v0,v1,v0
 10c:   3042ffff    andi    v0,v0,0xffff
 110:   30420fff    andi    v0,v0,0xfff
 114:   3045ffff    andi    a1,v0,0xffff
 118:   8fc40000    lw  a0,0(s8)
 11c:   0080182d    move    v1,a0
 120:   00031080    sll v0,v1,0x2
 124:   0040182d    move    v1,v0
 128:   00031080    sll v0,v1,0x2
 12c:   00431023    subu    v0,v0,v1
 130:   00441023    subu    v0,v0,a0
 134:   000211c0    sll v0,v0,0x7
 138:   00c21021    addu    v0,a2,v0
 13c:   00051d00    sll v1,a1,0x14
 140:   8c4512d0    lw  a1,4816(v0)                 <<<<< !
 144:   3c04000f    lui a0,0xf
 148:   3484ffff    ori a0,a0,0xffff
 14c:   00a42024    and a0,a1,a0
 150:   00831825    or  v1,a0,v1
 154:   ac4312d0    sw  v1,4816(v0)                 <<<<< !
 158:   8fc20000    lw  v0,0(s8)
 15c:   24420001    addiu   v0,v0,1
 160:   afc20000    sw  v0,0(s8)
 164:   8fc20000    lw  v0,0(s8)
 168:   2c420004    sltiu   v0,v0,4
 16c:   1440ffab    bnez    v0,1c <testfunction+0x1c>
 170:   00000000    nop
 174:   03c0e82d    move    sp,s8
 178:   8fbe0014    lw  s8,20(sp)
 17c:   27bd0018    addiu   sp,sp,24
 180:   03e00008    jr  ra
 184:   00000000    nop


GCC 4.8
=======
00000000 <testfunction>:
   0:   27bdffe8    addiu   sp,sp,-24
   4:   afbe0014    sw  s8,20(sp)
   8:   03a0f02d    move    s8,sp
   c:   afc40018    sw  a0,24(s8)
  10:   afc00000    sw  zero,0(s8)
  14:   0800005a    j   168 <testfunction+0x168>
  18:   00000000    nop
  1c:   8fc20018    lw  v0,24(s8)
  20:   8c431ab8    lw  v1,6840(v0)
  24:   3c020020    lui v0,0x20
  28:   00621024    and v0,v1,v0
  2c:   14400006    bnez    v0,48 <testfunction+0x48>
  30:   00000000    nop
  34:   8fc20018    lw  v0,24(s8)
  38:   8c421ab8    lw  v0,6840(v0)
  3c:   30420200    andi    v0,v0,0x200
  40:   10400018    beqz    v0,a4 <testfunction+0xa4>
  44:   00000000    nop
  48:   8fc20018    lw  v0,24(s8)
  4c:   8c431ab8    lw  v1,6840(v0)
  50:   3c020020    lui v0,0x20
  54:   00621024    and v0,v1,v0
  58:   1440000e    bnez    v0,94 <testfunction+0x94>
  5c:   00000000    nop
  60:   8fc20018    lw  v0,24(s8)
  64:   8c421ab8    lw  v0,6840(v0)
  68:   30420200    andi    v0,v0,0x200
  6c:   10400005    beqz    v0,84 <testfunction+0x84>
  70:   00000000    nop
  74:   24030fff    li  v1,4095
  78:   0000102d    move    v0,zero
  7c:   08000027    j   9c <testfunction+0x9c>
  80:   00000000    nop
  84:   2403ffff    li  v1,-1
  88:   2402ffff    li  v0,-1
  8c:   0800002c    j   b0 <testfunction+0xb0>
  90:   00000000    nop
  94:   240300ff    li  v1,255
  98:   0000102d    move    v0,zero
  9c:   0800002c    j   b0 <testfunction+0xb0>
  a0:   00000000    nop
  a4:   2403ffff    li  v1,-1
  a8:   3c027fff    lui v0,0x7fff
  ac:   3442ffff    ori v0,v0,0xffff
  b0:   afc3000c    sw  v1,12(s8)
  b4:   afc20008    sw  v0,8(s8)
  b8:   8fc20018    lw  v0,24(s8)
  bc:   8c461ac0    lw  a2,6848(v0)
  c0:   8fc20018    lw  v0,24(s8)
  c4:   8c451ac0    lw  a1,6848(v0)
  c8:   8fc40000    lw  a0,0(s8)
  cc:   0080182d    move    v1,a0
  d0:   00031080    sll v0,v1,0x2
  d4:   0040182d    move    v1,v0
  d8:   00031080    sll v0,v1,0x2
 dc:   00431023    subu    v0,v0,v1
  e0:   00441023    subu    v0,v0,a0
  e4:   000211c0    sll v0,v0,0x7
  e8:   00a21021    addu    v0,a1,v0
  ec:   8c4212d0    lw  v0,4816(v0)
  f0:   00021502    srl v0,v0,0x14
  f4:   3042ffff    andi    v0,v0,0xffff
  f8:   0040182d    move    v1,v0
  fc:   97c2000e    lhu v0,14(s8)
 100:   00021027    nor v0,zero,v0
 104:   3042ffff    andi    v0,v0,0xffff
 108:   00621024    and v0,v1,v0
 10c:   3042ffff    andi    v0,v0,0xffff
 110:   30420fff    andi    v0,v0,0xfff
 114:   3045ffff    andi    a1,v0,0xffff
 118:   8fc40000    lw  a0,0(s8)
 11c:   0080182d    move    v1,a0
 120:   00031080    sll v0,v1,0x2
 124:   0040182d    move    v1,v0
 128:   00031080    sll v0,v1,0x2
 12c:   00431023    subu    v0,v0,v1
 130:   00441023    subu    v0,v0,a0
 134:   000211c0    sll v0,v0,0x7
 138:   00c21021    addu    v0,a2,v0
 13c:   30a30fff    andi    v1,a1,0xfff
 140:   00031900    sll v1,v1,0x4
 144:   8c4512ce    lw  a1,4814(v0)              <<<<< ! 
 148:   3c04ffff    lui a0,0xffff
 14c:   3484000f    ori a0,a0,0xf
 150:   00a42024    and a0,a1,a0
 154:   00831825    or  v1,a0,v1
 158:   ac4312ce    sw  v1,4814(v0)              <<<<< !
 15c:   8fc20000    lw  v0,0(s8)
 160:   24420001    addiu   v0,v0,1
 164:   afc20000    sw  v0,0(s8)
 168:   8fc20000    lw  v0,0(s8)
 16c:   2c420004    sltiu   v0,v0,4
 170:   1440ffaa    bnez    v0,1c <testfunction+0x1c>
 174:   00000000    nop
 178:   03c0e82d    move    sp,s8
 17c:   8fbe0014    lw  s8,20(sp)
 180:   27bd0018    addiu   sp,sp,24
 184:   03e00008    jr  ra
 188:   00000000    nop


$ diff --side-by-side --suppress-common /tmp/46 /tmp/48 -W120 |less
  14:   08000059        j       164 <testfunction+0x164>   |      14:  
0800005a        j       168 <testfunction+0x168>
  7c:   08000023        j       8c <testfunction+0x8c>     |      7c:  
08000027        j       9c <testfunction+0x9c>
  8c:   08000027        j       9c <testfunction+0x9c>     |      8c:  
0800002c        j       b0 <testfunction+0xb0>
 13c:   00051d00        sll     v1,a1,0x14                 |     13c:  
30a30fff        andi    v1,a1,0xfff
 140:   8c4512d0        lw      a1,4816(v0)                |     140:  
00031900        sll     v1,v1,0x4
 144:   3c04000f        lui     a0,0xf                     |     144:  
8c4512ce        lw      a1,4814(v0)
 148:   3484ffff        ori     a0,a0,0xffff               |     148:  
3c04ffff        lui     a0,0xffff
 14c:   00a42024        and     a0,a1,a0                   |     14c:  
3484000f        ori     a0,a0,0xf
 150:   00831825        or      v1,a0,v1                   |     150:  
00a42024        and     a0,a1,a0
 154:   ac4312d0        sw      v1,4816(v0)                |     154:  
00831825        or      v1,a0,v1
 158:   8fc20000        lw      v0,0(s8)                   |     158:  
ac4312ce        sw      v1,4814(v0)
 15c:   24420001        addiu   v0,v0,1                    |     15c:  
8fc20000        lw      v0,0(s8)
 160:   afc20000        sw      v0,0(s8)                   |     160:  
24420001        addiu   v0,v0,1
 164:   8fc20000        lw      v0,0(s8)                   |     164:  
afc20000        sw      v0,0(s8)
 168:   2c420004        sltiu   v0,v0,4                    |     168:  
8fc20000        lw      v0,0(s8)
 16c:   1440ffab        bnez    v0,1c <testfunction+0x1c   |     16c:  
2c420004        sltiu   v0,v0,4
 170:   00000000        nop                                |     170:  
1440ffaa        bnez    v0,1c <testfunction+0x1c
 174:   03c0e82d        move    sp,s8                      |     174:  
00000000        nop
 178:   8fbe0014        lw      s8,20(sp)                  |     178:  
03c0e82d        move    sp,s8
 17c:   27bd0018        addiu   sp,sp,24                   |     17c:  
8fbe0014        lw      s8,20(sp)
 180:   03e00008        jr      ra                         |     180:  
27bd0018        addiu   sp,sp,24
 184:   00000000        nop                                |     184:  
03e00008        jr      ra
                                                           >     188:  
00000000        nop

[Bug c/55882] unaligned load/store : incorrect struct offset

[jan.smets@alcatel-lucent.com]

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

Jan Smets <jan.smets@alcatel-lucent.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|                            |mips*

--- Comment #1 from Jan Smets <jan.smets@alcatel-lucent.com> 2013-01-05 01:43:46 UTC ---
Mips isa lvl 2, o32 
Optimizations off

[Bug c/55882] unaligned load/store : incorrect struct offset

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2013-01-07
     Ever Confirmed|0                           |1

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> 2013-01-07 10:09:58 UTC ---
Without a complete testcase it's impossible to tell what is going on.  If you
really cannot produce one you may be able to help by bisecting to a revision
that broke your test.

[Bug c/55882] unaligned load/store : incorrect struct offset

[jan.smets@alcatel-lucent.com]

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

--- Comment #3 from Jan Smets <jan.smets@alcatel-lucent.com> 2013-01-07 16:20:32 UTC ---


00000000 <testfunction>:                        00000000 <testfunction>:
...
  20:   03c21021  addu    v0,s8,v0          20:   03c21021  addu    v0,s8,v0

  24:   8c44000c  lw      a0,12(v0)     |   24:   8c44000a  lw      a0,10(v0)  

  28:   3c03000f  lui     v1,0xf        |   28:   3c03ffff  lui     v1,0xffff
  2c:   3463ffff  ori     v1,v1,0xffff  |   2c:   3463000f  ori     v1,v1,0xf
  30:   00831824  and     v1,a0,v1          30:   00831824  and     v1,a0,v1

  34:   ac43000c  sw      v1,12(v0)     |   34:   ac43000a  sw      v1,10(v0)   

  38:   03c0e82d  move    sp,s8             38:   03c0e82d  move    sp,s8
  3c:   8fbe03ec  lw      s8,1004(sp)       3c:   8fbe03ec  lw      s8,1004(sp)
  40:   27bd03f0  addiu   sp,sp,1008        40:   27bd03f0  addiu   sp,sp,1008
  44:   03e00008  jr      ra                44:   03e00008  jr      ra
  48:   00000000  nop                       48:   00000000  nop

[Bug c/55882] unaligned load/store : incorrect struct offset

[jan.smets@alcatel-lucent.com]

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

--- Comment #4 from Jan Smets <jan.smets@alcatel-lucent.com> 2013-01-07 16:22:17 UTC ---
Created attachment 29099
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29099
testcase-pr55882.c

[Bug c/55882] unaligned load/store : incorrect struct offset

[rguenth at gcc dot gnu.org]

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

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> 2013-01-08 15:07:16 UTC ---
(In reply to comment #3)
> 
> 00000000 <testfunction>:                        00000000 <testfunction>:
> ...
>   20:   03c21021  addu    v0,s8,v0          20:   03c21021  addu    v0,s8,v0
> 
>   24:   8c44000c  lw      a0,12(v0)     |   24:   8c44000a  lw      a0,10(v0)  
> 
>   28:   3c03000f  lui     v1,0xf        |   28:   3c03ffff  lui     v1,0xffff
>   2c:   3463ffff  ori     v1,v1,0xffff  |   2c:   3463000f  ori     v1,v1,0xf
>   30:   00831824  and     v1,a0,v1          30:   00831824  and     v1,a0,v1
> 
>   34:   ac43000c  sw      v1,12(v0)     |   34:   ac43000a  sw      v1,10(v0)   
> 
>   38:   03c0e82d  move    sp,s8             38:   03c0e82d  move    sp,s8
>   3c:   8fbe03ec  lw      s8,1004(sp)       3c:   8fbe03ec  lw      s8,1004(sp)
>   40:   27bd03f0  addiu   sp,sp,1008        40:   27bd03f0  addiu   sp,sp,1008
>   44:   03e00008  jr      ra                44:   03e00008  jr      ra
>   48:   00000000  nop                       48:   00000000  nop

I believe the right side is correct.  use_alt_rd_dqs is at offset 10
(enable_monitor at 0, step_out_pointer at 2, hold_out_pointer at 4, etc.).

Depending on enum behavior on mips (-fshort-enums?) mystruct is aligned
to two bytes.

Can you post -v output of the compile?  I'm trying to reproduce with
a cross to mips-elf now.

[Bug c/55882] unaligned load/store : incorrect struct offset

[rguenth at gcc dot gnu.org]

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

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> 2013-01-08 15:25:46 UTC ---
GCC clealy thinks that

(insn 21 20 0 2 (set (mem/j:SI (plus:SI (reg/f:SI 195)
                (const_int 10 [0xa])) [0+-2 S4 A32])
        (reg:SI 201)) t.i:85 -1

is 4-byte aligned (but it _does_ have this strange MEM_OFFSET setting ...
which should be useless without a MEM_EXPR).

Does

unsigned short
testfunction(void)
{
    mystruct dmfe[8];
    unsigned i = 0;
    dmfe[0].use_alt_rd_dqs = 1;
    dmfe[i].use_alt_rd_dqs = 0;
    return dmfe[0].use_alt_rd_dqs;
}

extern void abort (void);
int main () { if (testfunction() != 0) abort (); }

abort for you?

[Bug c/55882] unaligned load/store : incorrect struct offset

[rguenth at gcc dot gnu.org]

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

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> 2013-01-08 15:47:10 UTC ---
Ok with respect to alignment the issue is that when we expand_assignment
for to == dmfe[i_1].use_alt_rd_dqs we fall into the old trap for
STRICT_ALIGNMENT targets to use the mode alignment.  That's because
we fall into

          misalignp = false;
          to_rtx = expand_expr (tem, NULL_RTX, VOIDmode, EXPAND_WRITE);

which creates

(mem/c:BLK (plus:SI (reg/f:SI 189 virtual-stack-vars)
        (const_int 4 [0x4])) [0 dmfe+0 S992 A32])

as dmfe is 4-byte aligned.  With offset = (sizetype) i_1 * 124 + 2
and bitregion_start = 0, bitregion_end = 63, bitpos = 48 and after
adjusting the offset we feed

(mem/c:BLK (plus:SI (reg/f:SI 206)
        (const_int 6 [0x6])) [0 dmfe A16])

into set_mem_attributes_minus_bitpos (with bitpos == 48) and
get_object_alignment returns 32.  But we fail to consider bitpos
when using that alignment.

So ... does the following patch fix it for you?

Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c      (revision 195014)
+++ gcc/emit-rtl.c      (working copy)
@@ -1839,7 +1839,13 @@ set_mem_attributes_minus_bitpos (rtx ref

       if (!align_computed)
        {
-         unsigned int obj_align = get_object_alignment (t);
+         unsigned int obj_align;
+         unsigned HOST_WIDE_INT obj_bitpos;
+         get_object_alignment_1 (t, &obj_align, &obj_bitpos);
+         if (apply_bitpos)
+           obj_bitpos += apply_bitpos;
+         if (obj_bitpos != 0)
+           obj_align = (obj_bitpos & -obj_bitpos);
          attrs.align = MAX (attrs.align, obj_align);
        }
     }

[Bug c/55882] unaligned load/store : incorrect struct offset

[rguenth at gcc dot gnu.org]

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

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> 2013-01-08 15:54:46 UTC ---
Or more correct

Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c      (revision 195014)
+++ gcc/emit-rtl.c      (working copy)
@@ -1839,7 +1839,12 @@ set_mem_attributes_minus_bitpos (rtx ref

       if (!align_computed)
        {
-         unsigned int obj_align = get_object_alignment (t);
+         unsigned int obj_align;
+         unsigned HOST_WIDE_INT obj_bitpos;
+         get_object_alignment_1 (t, &obj_align, &obj_bitpos);
+         obj_bitpos = (obj_bitpos + apply_bitpos) & (obj_align - 1);
+         if (obj_bitpos != 0)
+           obj_align = (obj_bitpos & -obj_bitpos);
          attrs.align = MAX (attrs.align, obj_align);
        }
     }

[Bug c/55882] unaligned load/store : incorrect struct offset

[jan.smets@alcatel-lucent.com]

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

--- Comment #9 from Jan Smets <jan.smets@alcatel-lucent.com> 2013-01-08 21:45:23 UTC ---
Patch verified.

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

[rguenth at gcc dot gnu.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 ...

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

[rguenth at gcc dot gnu.org]

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

--- Comment #11 from Richard Biener <rguenth at gcc dot gnu.org> 2013-01-09 11:26:10 UTC ---
(In reply to comment #8)
> Or more correct
> 
> Index: gcc/emit-rtl.c
> ===================================================================
> --- gcc/emit-rtl.c      (revision 195014)
> +++ gcc/emit-rtl.c      (working copy)
> @@ -1839,7 +1839,12 @@ set_mem_attributes_minus_bitpos (rtx ref
> 
>        if (!align_computed)
>         {
> -         unsigned int obj_align = get_object_alignment (t);
> +         unsigned int obj_align;
> +         unsigned HOST_WIDE_INT obj_bitpos;
> +         get_object_alignment_1 (t, &obj_align, &obj_bitpos);
> +         obj_bitpos = (obj_bitpos + apply_bitpos) & (obj_align - 1);

And actually

         obj_bitpos = (obj_bitpos - apply_bitpos) & (obj_align - 1);

> +         if (obj_bitpos != 0)
> +           obj_align = (obj_bitpos & -obj_bitpos);
>           attrs.align = MAX (attrs.align, obj_align);
>         }
>      }

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

[rguenth at gcc dot gnu.org]

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

--- Comment #12 from Richard Biener <rguenth at gcc dot gnu.org> 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.

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

[jamborm at gcc dot gnu.org]

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

--- Comment #13 from Martin Jambor <jamborm at gcc dot gnu.org> 2013-01-10 09:48:41 UTC ---
I have bootstrapped and tested the patch from comment #11 on
sparc64-linux (gcc63 on compile farm) and there were no issues
(actually g++.old-deja/g++.law/operators23.C failed because ld
returned 2 but that is almost certainly noise).

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

[rguenth at gcc dot gnu.org]

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

--- Comment #14 from Richard Biener <rguenth at gcc dot gnu.org> 2013-01-15 12:48:28 UTC ---
Author: rguenth
Date: Tue Jan 15 12:48:13 2013
New Revision: 195194

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=195194
Log:
2013-01-15  Richard Biener  <rguenther@suse.de>

    PR middle-end/55882
    * emit-rtl.c (set_mem_attributes_minus_bitpos): Correctly
    account for bitpos when computing alignment.

    * gcc.dg/torture/pr55882.c: New testcase.

Added:
    trunk/gcc/testsuite/gcc.dg/torture/pr55882.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/emit-rtl.c
    trunk/gcc/testsuite/ChangeLog

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

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2
      Known to work|                            |4.8.0
            Summary|[4.7/4.8 Regression]        |[4.7 Regression] unaligned
                   |unaligned load/store :      |load/store : incorrect
                   |incorrect struct offset     |struct offset
      Known to fail|                            |4.7.2

--- Comment #15 from Richard Biener <rguenth at gcc dot gnu.org> 2013-01-15 12:53:40 UTC ---
Fixed on trunk sofar.

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

[rguenth at gcc dot gnu.org]

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

--- Comment #16 from Richard Biener <rguenth at gcc dot gnu.org> 2013-01-16 09:26:11 UTC ---
Author: rguenth
Date: Wed Jan 16 09:26:05 2013
New Revision: 195232

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=195232
Log:
2013-01-16  Richard Biener  <rguenther@suse.de>

    PR middle-end/55882
    * emit-rtl.c (set_mem_attributes_minus_bitpos): Correctly
    account for bitpos when computing alignment.

    * gcc.dg/torture/pr55882.c: New testcase.

Added:
    branches/gcc-4_7-branch/gcc/testsuite/gcc.dg/torture/pr55882.c
Modified:
    branches/gcc-4_7-branch/gcc/ChangeLog
    branches/gcc-4_7-branch/gcc/emit-rtl.c
    branches/gcc-4_7-branch/gcc/testsuite/ChangeLog

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

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #17 from Richard Biener <rguenth at gcc dot gnu.org> 2013-01-16 10:21:09 UTC ---
Fixed.