[PATCH; RFC] Alignment of component_ref's.

[Maxim Kuvyrkov <maxim at codesourcery dot com>]

Hi,

Below is proposed patch to fix poor compilation of the attached testcase 
for architectures, where large alignment of global variables is enforced 
(e.g., MIPS).

The issue is about suboptimal generation of references to packed struct.

Consider the attached testcase compiled for MIPS3 N32 at -O2 (other 
optimization levels produce basically the same code):

test.c: good():
===================================
good:
        .set    nomips16
        .frame  $sp,16,$31              # vars= 0, regs= 1/0, args= 0, 
gp= 0
        .mask   0x10000000,-8
        .fmask  0x00000000,0
        addiu   $sp,$sp,-16
        sd      $28,8($sp)
        lui     $28,%hi(%neg(%gp_rel(good)))
        addu    $28,$28,$25
        addiu   $28,$28,%lo(%neg(%gp_rel(good)))
        li      $2,86                   # 0x56
        li      $3,52                   # 0x34
        sb      $2,4($4)
        sb      $3,0($4)
        li      $2,18                   # 0x12
        li      $3,120                  # 0x78
        sb      $2,1($4)
        sb      $3,3($4)
        ld      $28,8($sp)
        .set    noreorder
        .set    nomacro
        j       $31
        addiu   $sp,$sp,16
        .set    macro
        .set    reorder

        .end    good

        .comm   v,30,8

test.c: bad():
===================================
bad:
        .set    nomips16
        .frame  $sp,16,$31              # vars= 0, regs= 1/0, args= 0, 
gp= 0
        .mask   0x10000000,-8
        .fmask  0x00000000,0
        addiu   $sp,$sp,-16
        sd      $28,8($sp)
        lui     $28,%hi(%neg(%gp_rel(bad)))
        addu    $28,$28,$25
        addiu   $28,$28,%lo(%neg(%gp_rel(bad)))
        la      $5,v
        li      $2,4660
        sh      $2,0($5)
        li      $2,-65536                       # 0xffffffffffff0000
        ld      $4,0($5)
        ori     $2,$2,0x1
        dsll    $2,$2,24
        daddiu  $2,$2,-1
        li      $3,2767                 # 0xacf
        and     $4,$4,$2
        dsll    $3,$3,27
        or      $4,$4,$3
        sd      $4,0($5)
        ld      $28,8($sp)
        .set    noreorder
        .set    nomacro
        j       $31
        addiu   $sp,$sp,16
        .set    macro
        .set    reorder

        .end    bad

        .comm   v,30,8


The problem is that references to field inherit 64-bit alignment from 
the array variable, which is global, and hence aligned at 8-bytes.  So 
GCC has nothing to do, but to load values at 8-byte boundary and then 
extract appropriate parts from those.  This issue is further worsen by 
the fact that 8-byte values are referenced with 2 4-byte references.

For the first reference - v[0] - everything plays well as extraction can 
be optimized away; for reference to v[1] we get the problem.

The proposed fix is to use field alignment when reference is shifted 
(bitpos != 0), given that it is lesser then the alignment we previously 
used.

This issue doesn't appear on other architectures (e.g., ARM) due to 
global variables not being aligned so rigorously.

I'm not sure if this patch can cause miscompilations or degrade code 
quality.  Comments?

FWIW, resulting code for the testcase is now rather optimal: we use word 
references for v[0] and byte references for v[1]:

bad:
        .set    nomips16
        .frame  $sp,16,$31              # vars= 0, regs= 1/0, args= 0, 
gp= 0
        .mask   0x10000000,-8
        .fmask  0x00000000,0
        addiu   $sp,$sp,-16
        sd      $28,8($sp)
        lui     $28,%hi(%neg(%gp_rel(bad)))
        addu    $28,$28,$25
        addiu   $28,$28,%lo(%neg(%gp_rel(bad)))
        la      $4,v
        li      $2,86                   # 0x56
        sb      $2,4($4)                # <- v[1].a second byte
        li      $3,4660
        li      $2,120                  # 0x78
        sh      $3,0($4)                # <- v[0].a
        sb      $2,3($4)                # <- v[1].a first byte
        ld      $28,8($sp)
        .set    noreorder
        .set    nomacro
        j       $31
        addiu   $sp,$sp,16
        .set    macro
        .set    reorder

        .end    bad

        .comm   v,30,8

Bootstrapped on x86 with all default languages, testing on MIPS pending.

Any comments?  OK for trunk?


Thanks,

Maxim
struct test_s {
        short a;
        char  b;
} __attribute__ ((packed));

struct test_s v[10];

#ifdef BAD
void bad(void)
{
        v[0].a = 0x1234;
        v[1].a = 0x5678;
        return;
}
#else
void good(struct test_s x[10])
{
        x[0].a = 0x1234;
        x[1].a = 0x5678;
        return;
}
#endif

2008-05-05  Maxim Kuvyrkov  <maxim@codesourcery.com>

	* emit-rtl.c (set_mem_attributes_minus_bitpos): Fix align of
	component_ref's.

Index: emit-rtl.c
===================================================================
--- emit-rtl.c	(revision 134952)
+++ emit-rtl.c	(working copy)
@@ -1632,6 +1632,18 @@ set_mem_attributes_minus_bitpos (rtx ref
 	  expr = component_ref_for_mem_expr (t);
 	  offset = const0_rtx;
 	  apply_bitpos = bitpos;
+
+	  /* If the reference is with offset outstanding on T,
+	     use alignment of the field.  This is used to avoid
+	     suboptimal field references via referencing a larger value
+	     and then extracting pieces of it.  Instead, we prefer to
+	     generate several smaller references.
+
+	     Use original alignment, though, when bitpos == 0, because
+	     extraction should be optimized away in this case.  */
+	  if (apply_bitpos && align > DECL_ALIGN (TREE_OPERAND (expr, 1)))
+	    align = DECL_ALIGN (TREE_OPERAND (expr, 1));
+
 	  /* ??? Any reason the field size would be different than
 	     the size we got from the type?  */
 	}