[patch,loopiv] misaligned packed array memory access

[patch,loopiv] misaligned packed array memory access

[Christian BRUEL]

[-- Attachment #1: Type: text/plain, Size: 991 bytes --]

The attached test case reduces a misaligned field access from an array 
of packed structs. In this example compiled with -O2 the field 
wMaxPacketSize is accessed (on sh4) with a mov.w instruction although it 
is byte aligned.

The tree-ssa-loop ivopts did not check packed struct offsets indexed by 
an induction variable. Thus if the struct size is not aligned, the field 
  becomes unaligned after the first iteration even if it was aligned 
from the base of this structure.

This patch solves this problem by expanding may_be_unaligned_p to check 
that a loop carried offset is a multiple of the desired alignment.

This bug impacted STRICT_ALIGNMENT targets. I regtested on sh-superh-elf 
and sh4-linux-gnu.

Also bootstraped for i686-pc-linux-gnu.

Thanks you for any feedback and trunk approval if ok.

-c

2008-01-28  Christian Bruel  <christian.bruel@st.com>

	* tree-ssa-loop-ivopts.c (may_be_unaligned_p): check loop carried offset.
	* (loop_offset_multiple_of): new function.




[-- Attachment #2: packed_align.trunk.patch --]
[-- Type: text/plain, Size: 3124 bytes --]

Index: tree-ssa-loop-ivopts.c
===================================================================
--- tree-ssa-loop-ivopts.c	(revision 131825)
+++ tree-ssa-loop-ivopts.c	(working copy)
@@ -1378,10 +1378,13 @@
   return true;
 }
 
+static bool
+loop_offset_multiple_of (struct ivopts_data *, tree , tree);
+
 /* Returns true if memory reference REF may be unaligned.  */
 
 static bool
-may_be_unaligned_p (tree ref)
+may_be_unaligned_p (struct ivopts_data *data, tree ref)
 {
   tree base;
   tree base_type;
@@ -1405,11 +1408,18 @@
   base_type = TREE_TYPE (base);
   base_align = TYPE_ALIGN (base_type);
 
-  if (mode != BLKmode
-      && (base_align < GET_MODE_ALIGNMENT (mode)
+  if (mode != BLKmode) {
+      if (base_align < GET_MODE_ALIGNMENT (mode)
 	  || bitpos % GET_MODE_ALIGNMENT (mode) != 0
-	  || bitpos % BITS_PER_UNIT != 0))
-    return true;
+	  || bitpos % BITS_PER_UNIT != 0) 
+	return true;
+      
+      if (toffset && contains_packed_reference (ref)) {
+	tree al = build_int_cst (NULL_TREE, GET_MODE_ALIGNMENT (mode) / BITS_PER_UNIT);
+	if (!loop_offset_multiple_of (data, toffset, al))
+	  return true;
+      } 
+  }
 
   return false;
 }
@@ -1467,7 +1477,7 @@
     goto fail;
 
   if (STRICT_ALIGNMENT
-      && may_be_unaligned_p (base))
+      && may_be_unaligned_p (data, base))
     goto fail;
 
   base = unshare_expr (base);
@@ -2566,6 +2576,63 @@
     }
 }
 
+/* Returns true if OFFSET is always a multiple of alignment AL
+   even if loop carried.  */
+
+static bool
+loop_offset_multiple_of (struct ivopts_data *data, tree offset, tree al)
+{
+  double_int mul;
+
+  STRIP_NOPS (offset);
+
+  if (TREE_CODE (offset) == SSA_NAME) {
+    struct iv *civ = get_iv (data, offset);
+    if (!civ || constant_multiple_of (offset, al, &mul))
+      return true;
+  }
+
+  if (TREE_CODE (offset) == MULT_EXPR) {
+    tree op0 = TREE_OPERAND (offset, 0);
+    tree op1 = TREE_OPERAND (offset, 1);
+    STRIP_NOPS (op0);
+    STRIP_NOPS (op1);
+
+    if (TREE_CODE (op1) != INTEGER_CST)
+      return false;
+      
+    if (TREE_CODE (op0) == SSA_NAME) {
+      struct iv *civ = get_iv (data, op0);
+      tree type = TREE_TYPE (op1);
+      if (!civ)
+	return true;
+
+      tree val = fold_build2 (PLUS_EXPR, type, civ->base, 
+			      fold_build2 (MULT_EXPR, type, civ->step, op1));
+
+      if (constant_multiple_of (val, al, &mul))
+	return true;
+    }
+    else if (loop_offset_multiple_of (data, op0, al))
+      return true;
+  }
+
+  else if (TREE_CODE (offset) == PLUS_EXPR
+	   || TREE_CODE (offset) == MINUS_EXPR) {
+    tree op0 = TREE_OPERAND (offset, 0);
+    tree op1 = TREE_OPERAND (offset, 1);
+    if (loop_offset_multiple_of (data, op0, al) &&
+	loop_offset_multiple_of (data, op1, al))
+      return true;
+  }
+
+  else if (TREE_CODE (offset) == INTEGER_CST) {
+    return constant_multiple_of (offset, al, &mul);
+  }
+
+  return false;
+}
+
 /* If A is (TYPE) BA and B is (TYPE) BB, and the types of BA and BB have the
    same precision that is at least as wide as the precision of TYPE, stores
    BA to A and BB to B, and returns the type of BA.  Otherwise, returns the

[-- Attachment #3: example.bad.c --]
[-- Type: text/x-c, Size: 354 bytes --]

struct usb_interface_descriptor {
 unsigned short wMaxPacketSize;
  char e;
} __attribute__ ((packed));

struct usb_device {
 int devnum;
 struct usb_interface_descriptor if_desc[2];
};

extern void foo (unsigned short);

void usb_set_maxpacket(struct usb_device *dev, int n)
{
  int i;

  for(i=0; i<n;i++)
    foo(dev->if_desc[i].wMaxPacketSize);
}

Re: [patch,loopiv] misaligned packed array memory access

[Christian BRUEL]

[-- Attachment #1: Type: text/plain, Size: 1273 bytes --]

Adding a testcase to the patch for the testsuite.

(aborts on sh-superh-elf-gcc),

Regards

-c

2008-01-28  Christian Bruel  <christian.bruel@st.com

	* gcc.dg/packed-array.c: New testcase.


Christian BRUEL wrote:
> The attached test case reduces a misaligned field access from an array 
> of packed structs. In this example compiled with -O2 the field 
> wMaxPacketSize is accessed (on sh4) with a mov.w instruction although it 
> is byte aligned.
> 
> The tree-ssa-loop ivopts did not check packed struct offsets indexed by 
> an induction variable. Thus if the struct size is not aligned, the field 
>  becomes unaligned after the first iteration even if it was aligned from 
> the base of this structure.
> 
> This patch solves this problem by expanding may_be_unaligned_p to check 
> that a loop carried offset is a multiple of the desired alignment.
> 
> This bug impacted STRICT_ALIGNMENT targets. I regtested on sh-superh-elf 
> and sh4-linux-gnu.
> 
> Also bootstraped for i686-pc-linux-gnu.
> 
> Thanks you for any feedback and trunk approval if ok.
> 
> -c
> 
> 2008-01-28  Christian Bruel  <christian.bruel@st.com>
> 
>     * tree-ssa-loop-ivopts.c (may_be_unaligned_p): check loop carried 
> offset.
>     * (loop_offset_multiple_of): new function.
> 
> 
> 


[-- Attachment #2: packed-array.c --]
[-- Type: text/x-c, Size: 532 bytes --]

/* { dg-do run } */
/* { dg-options "-O2 -fno-inline" } */

struct usb_interface_descriptor {
 unsigned short wMaxPacketSize;
  char e;
} __attribute__ ((packed));

struct usb_device {
 int devnum;
 struct usb_interface_descriptor if_desc[2];
};

extern int printf (const char *, ...);

void foo (unsigned short a)
{
  printf ("%d\n", a);
}

struct usb_device ndev;

void usb_set_maxpacket(int n)
{
  int i;

  for(i=0; i<n;i++)
    foo((&ndev)->if_desc[i].wMaxPacketSize);
}

int
main()
{
  usb_set_maxpacket(2);
  return 0;
}

Re: [PING][patch,loopiv] misaligned packed array memory access

[Christian BRUEL]

http://gcc.gnu.org/ml/gcc-patches/2008-01/msg01357.html

A tree-ssa loop maintainer greatly appreciated

entered for the record in bugzilla PR36178, with a newer patch to fix a 
sh-linux build issue.

thanks

-c

Re: [PING][patch,loopiv] misaligned packed array memory access

[Zdenek Dvorak]

Hi,

> http://gcc.gnu.org/ml/gcc-patches/2008-01/msg01357.html
> 
> A tree-ssa loop maintainer greatly appreciated
> 
> entered for the record in bugzilla PR36178, with a newer patch to fix a 
> sh-linux build issue.

can you post this newer patch, please?  Put me in the CC, I will review
it.

Zdenek

Re: [PING][patch,loopiv] misaligned packed array memory access

[Christian BRUEL]

[-- Attachment #1: Type: text/plain, Size: 638 bytes --]

here it is, thanks!
-c


2008-01-28  Christian Bruel  <christian.bruel@st.com>

	* tree-ssa-loop-ivopts.c (may_be_unaligned_p): Check iv offset. 
   	* (loop_offset_multiple_of): New function.


2008-01-28  Christian Bruel  <christian.bruel@st.com>

	* gcc.dg/packed-array.c: New testcase.

Zdenek Dvorak wrote:
> Hi,
> 
> 
>>http://gcc.gnu.org/ml/gcc-patches/2008-01/msg01357.html
>>
>>A tree-ssa loop maintainer greatly appreciated
>>
>>entered for the record in bugzilla PR36178, with a newer patch to fix a 
>>sh-linux build issue.
> 
> 
> can you post this newer patch, please?  Put me in the CC, I will review
> it.
> 
> Zdenek
> 


[-- Attachment #2: packed_align.trunk.patch --]
[-- Type: text/plain, Size: 6575 bytes --]

Index: gcc/tree-ssa-loop-ivopts.c
===================================================================
--- gcc/tree-ssa-loop-ivopts.c	(revision 132282)
+++ gcc/tree-ssa-loop-ivopts.c	(working copy)
@@ -1391,10 +1391,131 @@
   return true;
 }
 
+/* If we can prove that TOP = cst * BOT for some constant cst,
+   store cst to MUL and return true.  Otherwise return false.
+   The returned value is always sign-extended, regardless of the
+   signedness of TOP and BOT.  */
+
+static bool
+constant_multiple_of (tree top, tree bot, double_int *mul)
+{
+  tree mby;
+  enum tree_code code;
+  double_int res, p0, p1;
+  unsigned precision = TYPE_PRECISION (TREE_TYPE (top));
+
+  STRIP_NOPS (top);
+  STRIP_NOPS (bot);
+
+  if (operand_equal_p (top, bot, 0))
+    {
+      *mul = double_int_one;
+      return true;
+    }
+
+  code = TREE_CODE (top);
+  switch (code)
+    {
+    case MULT_EXPR:
+      mby = TREE_OPERAND (top, 1);
+      if (TREE_CODE (mby) != INTEGER_CST)
+	return false;
+
+      if (!constant_multiple_of (TREE_OPERAND (top, 0), bot, &res))
+	return false;
+
+      *mul = double_int_sext (double_int_mul (res, tree_to_double_int (mby)),
+			      precision);
+      return true;
+
+    case PLUS_EXPR:
+    case MINUS_EXPR:
+      if (!constant_multiple_of (TREE_OPERAND (top, 0), bot, &p0)
+	  || !constant_multiple_of (TREE_OPERAND (top, 1), bot, &p1))
+	return false;
+
+      if (code == MINUS_EXPR)
+	p1 = double_int_neg (p1);
+      *mul = double_int_sext (double_int_add (p0, p1), precision);
+      return true;
+
+    case INTEGER_CST:
+      if (TREE_CODE (bot) != INTEGER_CST)
+	return false;
+
+      p0 = double_int_sext (tree_to_double_int (top), precision);
+      p1 = double_int_sext (tree_to_double_int (bot), precision);
+      if (double_int_zero_p (p1))
+	return false;
+      *mul = double_int_sext (double_int_sdivmod (p0, p1, FLOOR_DIV_EXPR, &res),
+			      precision);
+      return double_int_zero_p (res);
+
+    default:
+      return false;
+    }
+}
+
+/* Returns true if OFFSET is always a multiple of alignment AL
+   even if loop carried.  */
+
+static bool
+loop_offset_multiple_of (struct ivopts_data *data, tree offset, tree al)
+{
+  double_int mul;
+
+  STRIP_NOPS (offset);
+
+  if (TREE_CODE (offset) == SSA_NAME) {
+    if (constant_multiple_of (offset, al, &mul))
+      return true;
+  }
+
+  if (TREE_CODE (offset) == MULT_EXPR) {
+    tree op0 = TREE_OPERAND (offset, 0);
+    tree op1 = TREE_OPERAND (offset, 1);
+    STRIP_NOPS (op0);
+    STRIP_NOPS (op1);
+
+    if (TREE_CODE (op1) != INTEGER_CST)
+      return false;
+      
+    if (TREE_CODE (op0) == SSA_NAME) {
+      struct iv *civ = get_iv (data, op0);
+      tree type = TREE_TYPE (op1);
+      if (!civ || integer_zerop (civ->step))
+	return true;
+
+      tree val = fold_build2 (PLUS_EXPR, type, civ->base, 
+			      fold_build2 (MULT_EXPR, type, civ->step, op1));
+
+      if (constant_multiple_of (val, al, &mul))
+	return true;
+    }
+    else if (loop_offset_multiple_of (data, op0, al))
+      return true;
+  }
+
+  else if (TREE_CODE (offset) == PLUS_EXPR
+	   || TREE_CODE (offset) == MINUS_EXPR) {
+    tree op0 = TREE_OPERAND (offset, 0);
+    tree op1 = TREE_OPERAND (offset, 1);
+    if (loop_offset_multiple_of (data, op0, al) &&
+	loop_offset_multiple_of (data, op1, al))
+      return true;
+  }
+
+  else if (TREE_CODE (offset) == INTEGER_CST) {
+    return constant_multiple_of (offset, al, &mul);
+  }
+
+  return false;
+}
+
 /* Returns true if memory reference REF may be unaligned.  */
 
 static bool
-may_be_unaligned_p (tree ref)
+may_be_unaligned_p (struct ivopts_data *data, tree ref)
 {
   tree base;
   tree base_type;
@@ -1418,11 +1539,18 @@
   base_type = TREE_TYPE (base);
   base_align = TYPE_ALIGN (base_type);
 
-  if (mode != BLKmode
-      && (base_align < GET_MODE_ALIGNMENT (mode)
+  if (mode != BLKmode) {
+      if (base_align < GET_MODE_ALIGNMENT (mode)
 	  || bitpos % GET_MODE_ALIGNMENT (mode) != 0
-	  || bitpos % BITS_PER_UNIT != 0))
-    return true;
+	  || bitpos % BITS_PER_UNIT != 0) 
+	return true;
+      
+      if (toffset && contains_packed_reference (ref)) {
+	tree al = build_int_cst (NULL_TREE, GET_MODE_ALIGNMENT (mode) / BITS_PER_UNIT);
+	if (!loop_offset_multiple_of (data, toffset, al))
+	  return true;
+      } 
+  }
 
   return false;
 }
@@ -1480,7 +1608,7 @@
     goto fail;
 
   if (STRICT_ALIGNMENT
-      && may_be_unaligned_p (base))
+      && may_be_unaligned_p (data, base))
     goto fail;
 
   base = unshare_expr (base);
@@ -2569,71 +2697,6 @@
   return (w >> bitno) & 1;
 }
 
-/* If we can prove that TOP = cst * BOT for some constant cst,
-   store cst to MUL and return true.  Otherwise return false.
-   The returned value is always sign-extended, regardless of the
-   signedness of TOP and BOT.  */
-
-static bool
-constant_multiple_of (tree top, tree bot, double_int *mul)
-{
-  tree mby;
-  enum tree_code code;
-  double_int res, p0, p1;
-  unsigned precision = TYPE_PRECISION (TREE_TYPE (top));
-
-  STRIP_NOPS (top);
-  STRIP_NOPS (bot);
-
-  if (operand_equal_p (top, bot, 0))
-    {
-      *mul = double_int_one;
-      return true;
-    }
-
-  code = TREE_CODE (top);
-  switch (code)
-    {
-    case MULT_EXPR:
-      mby = TREE_OPERAND (top, 1);
-      if (TREE_CODE (mby) != INTEGER_CST)
-	return false;
-
-      if (!constant_multiple_of (TREE_OPERAND (top, 0), bot, &res))
-	return false;
-
-      *mul = double_int_sext (double_int_mul (res, tree_to_double_int (mby)),
-			      precision);
-      return true;
-
-    case PLUS_EXPR:
-    case MINUS_EXPR:
-      if (!constant_multiple_of (TREE_OPERAND (top, 0), bot, &p0)
-	  || !constant_multiple_of (TREE_OPERAND (top, 1), bot, &p1))
-	return false;
-
-      if (code == MINUS_EXPR)
-	p1 = double_int_neg (p1);
-      *mul = double_int_sext (double_int_add (p0, p1), precision);
-      return true;
-
-    case INTEGER_CST:
-      if (TREE_CODE (bot) != INTEGER_CST)
-	return false;
-
-      p0 = double_int_sext (tree_to_double_int (top), precision);
-      p1 = double_int_sext (tree_to_double_int (bot), precision);
-      if (double_int_zero_p (p1))
-	return false;
-      *mul = double_int_sext (double_int_sdivmod (p0, p1, FLOOR_DIV_EXPR, &res),
-			      precision);
-      return double_int_zero_p (res);
-
-    default:
-      return false;
-    }
-}
-
 /* If A is (TYPE) BA and B is (TYPE) BB, and the types of BA and BB have the
    same precision that is at least as wide as the precision of TYPE, stores
    BA to A and BB to B, and returns the type of BA.  Otherwise, returns the

Re: [PING][patch,loopiv] misaligned packed array memory access

[Zdenek Dvorak]

Hi,

> 	* tree-ssa-loop-ivopts.c (may_be_unaligned_p): Check iv offset. 
>   	* (loop_offset_multiple_of): New function.

fix the changelog formating.

> +/* Returns true if OFFSET is always a multiple of alignment AL
> +   even if loop carried.  */
> +
> +static bool
> +loop_offset_multiple_of (struct ivopts_data *data, tree offset, tree al)

instead of this, it would perhaps be simpler to just add
constant_multiple_of check for the step of the induction variable
(after it is computed in find_interesting_uses_address)?

Zdenek

Re: [PING][patch,loopiv] misaligned packed array memory access

[Christian BRUEL]

I'm not sure, I tried to use only `constant_multiple_of` but didn't 
succeed without being over-conservative, because we want to check all 
conditions of alignments of base+step+offset. Furthermore we would need 
to duplicate most of the may_be_unaligned_p function (call to 
get_inner_reference).
I think that a walkthru of offset is needed anyway to get to the 
ssa_name if it exists, so I preferred to isolate it into 
'loop_offset_multiple_of'.

Regards,

Christian



Zdenek Dvorak wrote:
> Hi,
> 
> 
>>	* tree-ssa-loop-ivopts.c (may_be_unaligned_p): Check iv offset. 
>>  	* (loop_offset_multiple_of): New function.
> 
> 
> fix the changelog formating.
> 
> 
>>+/* Returns true if OFFSET is always a multiple of alignment AL
>>+   even if loop carried.  */
>>+
>>+static bool
>>+loop_offset_multiple_of (struct ivopts_data *data, tree offset, tree al)
> 
> 
> instead of this, it would perhaps be simpler to just add
> constant_multiple_of check for the step of the induction variable
> (after it is computed in find_interesting_uses_address)?
> 
> Zdenek
> 

Re: [PING][patch,loopiv] misaligned packed array memory access

[Zdenek Dvorak]

Hi,

> I'm not sure, I tried to use only `constant_multiple_of` but didn't 
> succeed without being over-conservative, because we want to check all 
> conditions of alignments of base+step+offset. Furthermore we would need 
> to duplicate most of the may_be_unaligned_p function (call to 
> get_inner_reference).

no, that is not what I ment.  What I had in mind is something like
the following patch (since we compute the step of the reference anyway,
duplicating that in loop_offset_multiple_of seems redundant):

Index: tree-ssa-loop-ivopts.c
===================================================================
*** tree-ssa-loop-ivopts.c	(revision 132341)
--- tree-ssa-loop-ivopts.c	(working copy)
*************** idx_record_use (tree base, tree *idx,
*** 1391,1400 ****
    return true;
  }
  
! /* Returns true if memory reference REF may be unaligned.  */
  
  static bool
! may_be_unaligned_p (tree ref)
  {
    tree base;
    tree base_type;
--- 1391,1400 ----
    return true;
  }
  
! /* Returns true if memory reference REF with step STEP may be unaligned.  */
  
  static bool
! may_be_unaligned_p (tree ref, tree step)
  {
    tree base;
    tree base_type;
*************** may_be_unaligned_p (tree ref)
*** 1404,1409 ****
--- 1404,1410 ----
    enum machine_mode mode;
    int unsignedp, volatilep;
    unsigned base_align;
+   double_int mul;
  
    /* TARGET_MEM_REFs are translated directly to valid MEMs on the target,
       thus they are not misaligned.  */
*************** may_be_unaligned_p (tree ref)
*** 1424,1429 ****
--- 1425,1433 ----
  	  || bitpos % BITS_PER_UNIT != 0))
      return true;
  
+   if (!constant_multiple_of (step, build_int_cst (TREE_TYPE (step), GET_MODE_ALIGNMENT (mode)), &mul))
+     return true;
+ 
    return false;
  }
  
*************** find_interesting_uses_address (struct iv
*** 1549,1555 ****
  
        /* Moreover, on strict alignment platforms, check that it is
  	 sufficiently aligned.  */
!       if (STRICT_ALIGNMENT && may_be_unaligned_p (base))
  	goto fail;
  
        base = build_fold_addr_expr (base);
--- 1553,1559 ----
  
        /* Moreover, on strict alignment platforms, check that it is
  	 sufficiently aligned.  */
!       if (STRICT_ALIGNMENT && may_be_unaligned_p (base, step))
  	goto fail;
  
        base = build_fold_addr_expr (base);

Re: [PING][patch,loopiv] misaligned packed array memory access

[Christian BRUEL]

ok, I see now what you meant, just that we also need to pass the 
computed offset:

     val1 = fold_build2 (MULT_EXPR, type, step, toffset);
     val = fold_build2 (PLUS_EXPR, base_type, base, val1);

as the base from constant_multiple_of.

I'll see what this gives.

Rgds,
-c

Zdenek Dvorak wrote:
> Hi,
> 
> 
>>I'm not sure, I tried to use only `constant_multiple_of` but didn't 
>>succeed without being over-conservative, because we want to check all 
>>conditions of alignments of base+step+offset. Furthermore we would need 
>>to duplicate most of the may_be_unaligned_p function (call to 
>>get_inner_reference).
> 
> 
> no, that is not what I ment.  What I had in mind is something like
> the following patch (since we compute the step of the reference anyway,
> duplicating that in loop_offset_multiple_of seems redundant):
> 
> Index: tree-ssa-loop-ivopts.c
> ===================================================================
> *** tree-ssa-loop-ivopts.c	(revision 132341)
> --- tree-ssa-loop-ivopts.c	(working copy)
> *************** idx_record_use (tree base, tree *idx,
> *** 1391,1400 ****
>     return true;
>   }
>   
> ! /* Returns true if memory reference REF may be unaligned.  */
>   
>   static bool
> ! may_be_unaligned_p (tree ref)
>   {
>     tree base;
>     tree base_type;
> --- 1391,1400 ----
>     return true;
>   }
>   
> ! /* Returns true if memory reference REF with step STEP may be unaligned.  */
>   
>   static bool
> ! may_be_unaligned_p (tree ref, tree step)
>   {
>     tree base;
>     tree base_type;
> *************** may_be_unaligned_p (tree ref)
> *** 1404,1409 ****
> --- 1404,1410 ----
>     enum machine_mode mode;
>     int unsignedp, volatilep;
>     unsigned base_align;
> +   double_int mul;
>   
>     /* TARGET_MEM_REFs are translated directly to valid MEMs on the target,
>        thus they are not misaligned.  */
> *************** may_be_unaligned_p (tree ref)
> *** 1424,1429 ****
> --- 1425,1433 ----
>   	  || bitpos % BITS_PER_UNIT != 0))
>       return true;
>   
> +   if (!constant_multiple_of (step, build_int_cst (TREE_TYPE (step), GET_MODE_ALIGNMENT (mode)), &mul))
> +     return true;
> + 
>     return false;
>   }
>   
> *************** find_interesting_uses_address (struct iv
> *** 1549,1555 ****
>   
>         /* Moreover, on strict alignment platforms, check that it is
>   	 sufficiently aligned.  */
> !       if (STRICT_ALIGNMENT && may_be_unaligned_p (base))
>   	goto fail;
>   
>         base = build_fold_addr_expr (base);
> --- 1553,1559 ----
>   
>         /* Moreover, on strict alignment platforms, check that it is
>   	 sufficiently aligned.  */
> !       if (STRICT_ALIGNMENT && may_be_unaligned_p (base, step))
>   	goto fail;
>   
>         base = build_fold_addr_expr (base);
> 

Re: [PING][patch,loopiv] misaligned packed array memory access

[Zdenek Dvorak]

Hi,

> ok, I see now what you meant, just that we also need to pass the 
> computed offset:
> 
>     val1 = fold_build2 (MULT_EXPR, type, step, toffset);

this does not seem to be necessary (step is the step of the
address of the reference in bytes, no need to multiply it by anything).

>     val = fold_build2 (PLUS_EXPR, base_type, base, val1);

I am not sure what you mean by this?  As far as I understand the
problem, we need to check that

1) the address is aligned in the first iteration -- which we do now, and
2) the step is a multiple of the alignment

Zdenek

Re: [PING][patch,loopiv] misaligned packed array memory access

[Christian BRUEL]

[-- Attachment #1: Type: text/plain, Size: 1271 bytes --]

Zdenek Dvorak wrote:
> Hi,
> 
> 
>>ok, I see now what you meant, just that we also need to pass the 
>>computed offset:
>>
>>    val1 = fold_build2 (MULT_EXPR, type, step, toffset);
> 
> 
> this does not seem to be necessary (step is the step of the
> address of the reference in bytes, no need to multiply it by anything).
> 
> 
>>    val = fold_build2 (PLUS_EXPR, base_type, base, val1);
> 
> 
> I am not sure what you mean by this?  As far as I understand the
> problem, we need to check that
> 
> 1) the address is aligned in the first iteration -- which we do now, and
> 2) the step is a multiple of the alignment
> 
> Zdenek
> 

yes you are right. in fact my original patch was developped against a 
4.2 branch were `may_be_unaligned_p` was called before 'step' was computed.

Well, here is a revisited patch against the mainline using your 
suggestion. I passed the testsuite for sh-superh-elf and bootstraped the 
compiler for i686-pc-linux-gnu.

2008-01-28  Christian Bruel  <christian.bruel@st.com>
             Zdenek Dvorak  <ook@ucw.cz>
	
         * tree-ssa-loop-ivopts.c (may_be_unaligned_p): Check step 
alignment.

2008-01-28  Christian Bruel  <christian.bruel@st.com>

         * gcc.dg/packed-array.c: New testcase.

thanks for your help,

Christian



[-- Attachment #2: packed_align.trunk.patch --]
[-- Type: text/plain, Size: 5316 bytes --]

Index: gcc/tree-ssa-loop-ivopts.c
===================================================================
--- gcc/tree-ssa-loop-ivopts.c	(revision 132388)
+++ gcc/tree-ssa-loop-ivopts.c	(working copy)
@@ -1391,11 +1391,76 @@
   return true;
 }
 
-/* Returns true if memory reference REF may be unaligned.  */
+/* If we can prove that TOP = cst * BOT for some constant cst,
+   store cst to MUL and return true.  Otherwise return false.
+   The returned value is always sign-extended, regardless of the
+   signedness of TOP and BOT.  */
 
 static bool
-may_be_unaligned_p (tree ref)
+constant_multiple_of (tree top, tree bot, double_int *mul)
 {
+  tree mby;
+  enum tree_code code;
+  double_int res, p0, p1;
+  unsigned precision = TYPE_PRECISION (TREE_TYPE (top));
+
+  STRIP_NOPS (top);
+  STRIP_NOPS (bot);
+
+  if (operand_equal_p (top, bot, 0))
+    {
+      *mul = double_int_one;
+      return true;
+    }
+
+  code = TREE_CODE (top);
+  switch (code)
+    {
+    case MULT_EXPR:
+      mby = TREE_OPERAND (top, 1);
+      if (TREE_CODE (mby) != INTEGER_CST)
+	return false;
+
+      if (!constant_multiple_of (TREE_OPERAND (top, 0), bot, &res))
+	return false;
+
+      *mul = double_int_sext (double_int_mul (res, tree_to_double_int (mby)),
+			      precision);
+      return true;
+
+    case PLUS_EXPR:
+    case MINUS_EXPR:
+      if (!constant_multiple_of (TREE_OPERAND (top, 0), bot, &p0)
+	  || !constant_multiple_of (TREE_OPERAND (top, 1), bot, &p1))
+	return false;
+
+      if (code == MINUS_EXPR)
+	p1 = double_int_neg (p1);
+      *mul = double_int_sext (double_int_add (p0, p1), precision);
+      return true;
+
+    case INTEGER_CST:
+      if (TREE_CODE (bot) != INTEGER_CST)
+	return false;
+
+      p0 = double_int_sext (tree_to_double_int (top), precision);
+      p1 = double_int_sext (tree_to_double_int (bot), precision);
+      if (double_int_zero_p (p1))
+	return false;
+      *mul = double_int_sext (double_int_sdivmod (p0, p1, FLOOR_DIV_EXPR, &res),
+			      precision);
+      return double_int_zero_p (res);
+
+    default:
+      return false;
+    }
+}
+
+/* Returns true if memory reference REF with step STEP may be unaligned.  */
+
+static bool
+may_be_unaligned_p (tree ref, tree step)
+{
   tree base;
   tree base_type;
   HOST_WIDE_INT bitsize;
@@ -1418,12 +1483,20 @@
   base_type = TREE_TYPE (base);
   base_align = TYPE_ALIGN (base_type);
 
-  if (mode != BLKmode
-      && (base_align < GET_MODE_ALIGNMENT (mode)
-	  || bitpos % GET_MODE_ALIGNMENT (mode) != 0
-	  || bitpos % BITS_PER_UNIT != 0))
-    return true;
+  if (mode != BLKmode) {
+    double_int mul;
+    tree al = build_int_cst (TREE_TYPE (step),
+			     GET_MODE_ALIGNMENT (mode) / BITS_PER_UNIT);
 
+    if (base_align < GET_MODE_ALIGNMENT (mode)
+	|| bitpos % GET_MODE_ALIGNMENT (mode) != 0
+	|| bitpos % BITS_PER_UNIT != 0)
+      return true;
+    
+    if (! constant_multiple_of (step, al, &mul))
+      return true;
+  }
+
   return false;
 }
 
@@ -1549,7 +1622,7 @@
 
       /* Moreover, on strict alignment platforms, check that it is
 	 sufficiently aligned.  */
-      if (STRICT_ALIGNMENT && may_be_unaligned_p (base))
+      if (STRICT_ALIGNMENT && may_be_unaligned_p (base, step))
 	goto fail;
 
       base = build_fold_addr_expr (base);
@@ -2585,71 +2658,6 @@
   return (w >> bitno) & 1;
 }
 
-/* If we can prove that TOP = cst * BOT for some constant cst,
-   store cst to MUL and return true.  Otherwise return false.
-   The returned value is always sign-extended, regardless of the
-   signedness of TOP and BOT.  */
-
-static bool
-constant_multiple_of (tree top, tree bot, double_int *mul)
-{
-  tree mby;
-  enum tree_code code;
-  double_int res, p0, p1;
-  unsigned precision = TYPE_PRECISION (TREE_TYPE (top));
-
-  STRIP_NOPS (top);
-  STRIP_NOPS (bot);
-
-  if (operand_equal_p (top, bot, 0))
-    {
-      *mul = double_int_one;
-      return true;
-    }
-
-  code = TREE_CODE (top);
-  switch (code)
-    {
-    case MULT_EXPR:
-      mby = TREE_OPERAND (top, 1);
-      if (TREE_CODE (mby) != INTEGER_CST)
-	return false;
-
-      if (!constant_multiple_of (TREE_OPERAND (top, 0), bot, &res))
-	return false;
-
-      *mul = double_int_sext (double_int_mul (res, tree_to_double_int (mby)),
-			      precision);
-      return true;
-
-    case PLUS_EXPR:
-    case MINUS_EXPR:
-      if (!constant_multiple_of (TREE_OPERAND (top, 0), bot, &p0)
-	  || !constant_multiple_of (TREE_OPERAND (top, 1), bot, &p1))
-	return false;
-
-      if (code == MINUS_EXPR)
-	p1 = double_int_neg (p1);
-      *mul = double_int_sext (double_int_add (p0, p1), precision);
-      return true;
-
-    case INTEGER_CST:
-      if (TREE_CODE (bot) != INTEGER_CST)
-	return false;
-
-      p0 = double_int_sext (tree_to_double_int (top), precision);
-      p1 = double_int_sext (tree_to_double_int (bot), precision);
-      if (double_int_zero_p (p1))
-	return false;
-      *mul = double_int_sext (double_int_sdivmod (p0, p1, FLOOR_DIV_EXPR, &res),
-			      precision);
-      return double_int_zero_p (res);
-
-    default:
-      return false;
-    }
-}
-
 /* If A is (TYPE) BA and B is (TYPE) BB, and the types of BA and BB have the
    same precision that is at least as wide as the precision of TYPE, stores
    BA to A and BB to B, and returns the type of BA.  Otherwise, returns the

[-- Attachment #3: packed-array.c --]
[-- Type: text/x-c, Size: 487 bytes --]

/* { dg-do run } */
/* { dg-options "-O2 -fno-inline" } */

struct usb_interface_descriptor {
 unsigned short wMaxPacketSize;
  char e;
} __attribute__ ((packed));

struct usb_device {
 int devnum;
 struct usb_interface_descriptor if_desc[2];
};

int b;

void foo (unsigned short a)
{
  b = a;
}

struct usb_device ndev;

void usb_set_maxpacket(int n)
{
  int i;

  for(i=0; i<n;i++)
    foo((&ndev)->if_desc[i].wMaxPacketSize);
}

int
main()
{
  usb_set_maxpacket(2);
  return b;
}

Re: [PING][patch,loopiv] misaligned packed array memory access

[Zdenek Dvorak]

Hi,

> Well, here is a revisited patch against the mainline using your 
> suggestion. I passed the testsuite for sh-superh-elf and bootstraped the 
> compiler for i686-pc-linux-gnu.

the patch is ok,

Zdenek

Re: [PING][patch,loopiv] misaligned packed array memory access

[Maxim Kuvyrkov]

Zdenek Dvorak wrote:
> Hi,
> 
>> Well, here is a revisited patch against the mainline using your 
>> suggestion. I passed the testsuite for sh-superh-elf and bootstraped the 
>> compiler for i686-pc-linux-gnu.

Hello,

Can you consider backporting this patch to 4.3 branch?  I've seen GCC 
4.3 generating misaligned accesses due to the bug this patch fixes on 
MIPS and ARM.


Thanks,

Maxim