may_be_unaligned_p bug?

may_be_unaligned_p bug?

[DJ Delorie]

In tree-ssa-loop-ivopts.c:may_be_unaligned_p(), we call
get_inner_reference to figure out bitpos, but we don't take into
account toffset - which may make the reference less aligned than we
expect.  Is this supposed to be accounted for by STEP ?  It doesn't
always work with nested arrays - STEP is the innermost array, but
doesn't account for outer arrays.

struct packed_struct
{
  struct packed_struct1
  {
    unsigned char cc11;
    unsigned char cc12;
  } __attribute__ ((packed)) pst1;
  struct packed_struct2
  {
    unsigned char cc21;
    unsigned char cc22;
    unsigned short ss[104];
    unsigned char cc23[13];
  } __attribute__ ((packed)) pst2[4];
} __attribute__ ((packed));

typedef struct
{
  int ii;
  struct packed_struct buf;
} info_t;

int
foo (info_t * info)
{
  int i, j;

  for (i = 0; i < info->buf.pst1.cc11; i++)
    {
      for (j = 0; j < info->buf.pst2[i].cc22; j++)
        {
          dj1 (info->buf.pst2[i].ss[j]);
        }
    }

  return 0;
}


As a workaround, I had may_be_unaligned_p() return true if toffset is
set, but there's likely times when that results in suboptimal code.

Re: may_be_unaligned_p bug?

[Eric Botcazou]

> As a workaround, I had may_be_unaligned_p() return true if toffset is
> set, but there's likely times when that results in suboptimal code.

Wouldn't this be sufficient?

Index: tree-ssa-loop-ivopts.c
===================================================================
--- tree-ssa-loop-ivopts.c	(revision 158148)
+++ tree-ssa-loop-ivopts.c	(working copy)
@@ -1546,6 +1546,9 @@ may_be_unaligned_p (tree ref, tree step)
 	  || bitpos % BITS_PER_UNIT != 0)
 	return true;
 
+      if (toffset && !constant_multiple_of (toffset, al, &mul))
+	return true;
+
       if (!constant_multiple_of (step, al, &mul))
 	return true;
     }

-- 
Eric Botcazou

Re: may_be_unaligned_p bug?

[Eric Botcazou]

> Index: tree-ssa-loop-ivopts.c
> ===================================================================
> --- tree-ssa-loop-ivopts.c	(revision 158148)
> +++ tree-ssa-loop-ivopts.c	(working copy)
> @@ -1546,6 +1546,9 @@ may_be_unaligned_p (tree ref, tree step)
>
>  	  || bitpos % BITS_PER_UNIT != 0)
>
>  	return true;
>
> +      if (toffset && !constant_multiple_of (toffset, al, &mul))
> +	return true;
> +
>        if (!constant_multiple_of (step, al, &mul))
>  	return true;
>      }

constant_multiple_of is too restrictive though, something like:

Index: tree-ssa-loop-ivopts.c
===================================================================
--- tree-ssa-loop-ivopts.c	(revision 158148)
+++ tree-ssa-loop-ivopts.c	(working copy)
@@ -1537,16 +1537,18 @@ may_be_unaligned_p (tree ref, tree step)
 
   if (mode != BLKmode)
     {
-      double_int mul;
-      tree al = build_int_cst (TREE_TYPE (step),
-			       GET_MODE_ALIGNMENT (mode) / BITS_PER_UNIT);
+      unsigned mode_align = GET_MODE_ALIGNMENT (mode);
 
-      if (base_align < GET_MODE_ALIGNMENT (mode)
-	  || bitpos % GET_MODE_ALIGNMENT (mode) != 0
-	  || bitpos % BITS_PER_UNIT != 0)
+      if (base_align < mode_align
+	  || (bitpos % mode_align) != 0
+	  || (bitpos % BITS_PER_UNIT) != 0)
 	return true;
 
-      if (!constant_multiple_of (step, al, &mul))
+      if (toffset
+	  && (highest_pow2_factor (toffset) * BITS_PER_UNIT) < mode_align)
+	return true;
+
+      if ((highest_pow2_factor (step) * BITS_PER_UNIT) < mode_align)
 	return true;
     }
 
-- 
Eric Botcazou

Re: may_be_unaligned_p bug?

[DJ Delorie]

I can confirm this fixes my test case.  Had I known about
highest_pow2_factor() I might have come up with this myself ;-)

> Index: tree-ssa-loop-ivopts.c
> ===================================================================
> --- tree-ssa-loop-ivopts.c	(revision 158148)
> +++ tree-ssa-loop-ivopts.c	(working copy)
> @@ -1537,16 +1537,18 @@ may_be_unaligned_p (tree ref, tree step)
>  
>    if (mode != BLKmode)
>      {
> -      double_int mul;
> -      tree al = build_int_cst (TREE_TYPE (step),
> -			       GET_MODE_ALIGNMENT (mode) / BITS_PER_UNIT);
> +      unsigned mode_align = GET_MODE_ALIGNMENT (mode);
>  
> -      if (base_align < GET_MODE_ALIGNMENT (mode)
> -	  || bitpos % GET_MODE_ALIGNMENT (mode) != 0
> -	  || bitpos % BITS_PER_UNIT != 0)
> +      if (base_align < mode_align
> +	  || (bitpos % mode_align) != 0
> +	  || (bitpos % BITS_PER_UNIT) != 0)
>  	return true;
>  
> -      if (!constant_multiple_of (step, al, &mul))
> +      if (toffset
> +	  && (highest_pow2_factor (toffset) * BITS_PER_UNIT) < mode_align)
> +	return true;
> +
> +      if ((highest_pow2_factor (step) * BITS_PER_UNIT) < mode_align)
>  	return true;
>      }
>  

Re: may_be_unaligned_p bug?

[Eric Botcazou]

> I can confirm this fixes my test case.  Had I known about
> highest_pow2_factor() I might have come up with this myself ;-)

OK, I'll do some testing with it tomorrow.  Which GCC versions are affected?

-- 
Eric Botcazou

Re: may_be_unaligned_p bug?

[DJ Delorie]

> OK, I'll do some testing with it tomorrow.  Which GCC versions are affected?

I tested trunk and an old 4.2.1 internal branch, and found the bug on
both, so anything in between would be affected too.  It goes back at
least to this patch, which mostly fixed the bug:

2008-02-19  Christian Bruel  <christian.bruel@st.com>
            Zdenek Dvorak  <ook@ucw.cz>

	* tree-ssa-loop-ivopts.c (may_be_unaligned_p): Check step alignment.

(our 4.2.1 branch, plus the above patch, plus your patch, works)

Re: may_be_unaligned_p bug?

[Eric Botcazou]

> I tested trunk and an old 4.2.1 internal branch, and found the bug on
> both, so anything in between would be affected too.  It goes back at
> least to this patch, which mostly fixed the bug:
>
> 2008-02-19  Christian Bruel  <christian.bruel@st.com>
>             Zdenek Dvorak  <ook@ucw.cz>
>
> 	* tree-ssa-loop-ivopts.c (may_be_unaligned_p): Check step alignment.

OK, I tested it (a little) with trunk on SPARC/Solaris and SPARC64/Solaris and 
things look good as well.  I won't be able to do a formal submission before 
at least one week so if you feel like beating me to it, go ahead.

-- 
Eric Botcazou