RF[DA]: fix execute/931004-10.c regressions

RF[DA]: fix execute/931004-10.c regressions

[Joern Rennecke]

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


-- 
--------------------------
SuperH
2430 Aztec West / Almondsbury / BRISTOL / BS32 4AQ
T:+44 1454 462330

[-- Attachment #2: arg-align-fix --]
[-- Type: text/plain, Size: 2712 bytes --]

execute/931004-10.c fails at -O0 for the Sh64 subtargets
-m5-32media-nofpu, -m5-64media, -m5-64media-nofpu and -m5.
It fails at -O0, -O1, -O2, -O3 -fomit-frame-pointer,
-O3 -fomit-frame-pointer, -O3 -fomit-frame-pointer -funroll-all-loops
-finline-functions, -O3 -g and -Os for -m5-compact and -m5-compact-nofpu,

Looking at the -m5 -O0 case, I see that the third argument in the call to
f, x[1], is loaded with a quad word load, and this fails due to misalignment.

The word loaded for x[1] is:
(mem/s/j:DI (reg/f:SI 167) [0 x+2 S8 A64])
Before your change to set_mem_attributes on Sunday, the alignment according
to MEM_ALIGN was that of the component, x[1], i.e. 16.  Now it is that of
the underlying object, x, i.e. 64.
We don't have any documentation that spells out what MEM_ALIGN actually
means.  The new bahaviour is consistent inasmuch as before and after
your patch, the offset and size are 2 (the offset within x) and 8 (the
size of x), respectively, and expr is x.
However, it is not what extract_fixed_bit_field expects.
It things MEM_ALIGN is the alignment of the memory access, and
passes this on to get_best_mode.
The following patch makes it compute the actual alignment of the
memory reference.

Tue Jul 23 20:28:18 2002  J"orn Rennecke <joern.rennecke@superh.com>

	* expmed.c (extract_fixed_bit_field): Take MEM_OFFSET into account.

Index: expmed.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/expmed.c,v
retrieving revision 1.122
diff -p -r1.122 expmed.c
*** expmed.c	24 Jun 2002 10:08:37 -0000	1.122
--- expmed.c	23 Jul 2002 19:28:18 -0000
*************** extract_fixed_bit_field (tmode, op0, off
*** 1589,1596 ****
  	 includes the entire field.  If such a mode would be larger than
  	 a word, we won't be doing the extraction the normal way.  */
  
        mode = get_best_mode (bitsize, bitpos + offset * BITS_PER_UNIT,
! 			    MEM_ALIGN (op0), word_mode, MEM_VOLATILE_P (op0));
  
        if (mode == VOIDmode)
  	/* The only way this should occur is if the field spans word
--- 1589,1604 ----
  	 includes the entire field.  If such a mode would be larger than
  	 a word, we won't be doing the extraction the normal way.  */
  
+       HOST_WIDE_INT align = MEM_ALIGN (op0);
+       HOST_WIDE_INT inner_offset
+ 	= MEM_OFFSET (op0) ? INTVAL (MEM_OFFSET (op0)) : 0;
+ 
+       if (inner_offset)
+ 	align = MIN (align,
+ 		     ((unsigned HOST_WIDE_INT) (inner_offset & -inner_offset)
+ 		      * BITS_PER_UNIT));
        mode = get_best_mode (bitsize, bitpos + offset * BITS_PER_UNIT,
! 			    align, word_mode, MEM_VOLATILE_P (op0));
  
        if (mode == VOIDmode)
  	/* The only way this should occur is if the field spans word

Re: RF[DA]: fix execute/931004-10.c regressions

[Richard Henderson]

On Tue, Jul 23, 2002 at 08:50:13PM +0100, Joern Rennecke wrote:
> Before your change to set_mem_attributes on Sunday, the alignment according
> to MEM_ALIGN was that of the component, x[1], i.e. 16.  Now it is that of
> the underlying object, x, i.e. 64.

Really?  That shouldn't have changed.  I'll look into it.

> We don't have any documentation that spells out what MEM_ALIGN actually
> means.

It's the alignment of the MEM, not the MEM_EXPR, or MEM_OFFSET or
whatever.


r~

Re: RF[DA]: fix execute/931004-10.c regressions

[Richard Henderson]

On Tue, Jul 23, 2002 at 08:50:13PM +0100, Joern Rennecke wrote:
> Before your change to set_mem_attributes on Sunday, the alignment according
> to MEM_ALIGN was that of the component, x[1], i.e. 16.  Now it is that of
> the underlying object, x, i.e. 64.

Here's the patch I'm testing on x86.

Twas some careless pasting of the DECL_P case from the 
COMPONENT_REF case into the ARRAY_REF case.


r~


	* emit-rtl.c (set_mem_attributes): Fix size and alignment thinkos
	in ARRAY_REF of DECL_P case.

Index: emit-rtl.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/emit-rtl.c,v
retrieving revision 1.286
diff -c -p -d -r1.286 emit-rtl.c
*** emit-rtl.c	22 Jul 2002 00:23:47 -0000	1.286
--- emit-rtl.c	24 Jul 2002 23:11:45 -0000
*************** set_mem_attributes (ref, t, objectp)
*** 1808,1819 ****
  	  if (DECL_P (t))
  	    {
  	      expr = t;
  	      if (host_integerp (off_tree, 1))
! 		offset = GEN_INT (tree_low_cst (off_tree, 1));
! 	      size = (DECL_SIZE_UNIT (t)
! 		      && host_integerp (DECL_SIZE_UNIT (t), 1)
! 		      ? GEN_INT (tree_low_cst (DECL_SIZE_UNIT (t), 1)) : 0);
! 	      align = DECL_ALIGN (t);
  	    }
  	  else if (TREE_CODE (t) == COMPONENT_REF)
  	    {
--- 1808,1823 ----
  	  if (DECL_P (t))
  	    {
  	      expr = t;
+ 	      offset = NULL;
  	      if (host_integerp (off_tree, 1))
! 		{
! 		  HOST_WIDE_INT ioff = tree_low_cst (off_tree, 1);
! 		  HOST_WIDE_INT aoff = (ioff & -ioff) * BITS_PER_UNIT;
! 		  align = DECL_ALIGN (t);
! 		  if (aoff && aoff < align)
! 	            align = aoff;
! 		  offset = GEN_INT (ioff);
! 		}
  	    }
  	  else if (TREE_CODE (t) == COMPONENT_REF)
  	    {

Re: RF[DA]: fix execute/931004-10.c regressions

[Joern Rennecke]

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

Richard Henderson wrote:
> 
> On Tue, Jul 23, 2002 at 08:50:13PM +0100, Joern Rennecke wrote:
> > Before your change to set_mem_attributes on Sunday, the alignment according
> > to MEM_ALIGN was that of the component, x[1], i.e. 16.  Now it is that of
> > the underlying object, x, i.e. 64.
> 
> Really?  That shouldn't have changed.  I'll look into it.
> 
> > We don't have any documentation that spells out what MEM_ALIGN actually
> > means.
> 
> It's the alignment of the MEM, not the MEM_EXPR, or MEM_OFFSET or
> whatever.

I've attached a patch document this.
	
-- 
--------------------------
SuperH
2430 Aztec West / Almondsbury / BRISTOL / BS32 4AQ
T:+44 1454 462330

[-- Attachment #2: mem_attr-doc --]
[-- Type: text/plain, Size: 1006 bytes --]

Thu Jul 25 10:32:35 2002  J"orn Rennecke <joern.rennecke@superh.com>

	* rtl.h (mem_attrs): Spell out more clearly the roles of ALIGN,
	DECL and OFFSET.

Index: rtl.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/rtl.h,v
retrieving revision 1.365
diff -p -r1.365 rtl.h
*** rtl.h	19 Jul 2002 23:11:18 -0000	1.365
--- rtl.h	25 Jul 2002 09:30:35 -0000
*************** typedef struct
*** 89,94 ****
--- 89,98 ----
     so MEMs that the same attributes share a data structure.  This means
     they cannot be modified in place.  If any element is nonzero, it means
     the value of the corresponding attribute is unknown.  */
+ /* ALIGN is the alignment of the MEM itself, while EXPR can describe a
+    larger underlying object, which might have a stricter alignment; SIZE
+    is the size of the object, and OFFSET is the offset of the MEM within
+    that object.  */
  typedef struct mem_attrs GTY(())
  {
    HOST_WIDE_INT alias;		/* Memory alias set.  */

Re: RF[DA]: fix execute/931004-10.c regressions

[Joern Rennecke]

Richard Henderson wrote:
> 
> On Tue, Jul 23, 2002 at 08:50:13PM +0100, Joern Rennecke wrote:
> > Before your change to set_mem_attributes on Sunday, the alignment according
> > to MEM_ALIGN was that of the component, x[1], i.e. 16.  Now it is that of
> > the underlying object, x, i.e. 64.
> 
> Here's the patch I'm testing on x86.
> 
> Twas some careless pasting of the DECL_P case from the
> COMPONENT_REF case into the ARRAY_REF case.
> 
> r~
> 
>         * emit-rtl.c (set_mem_attributes): Fix size and alignment thinkos
>         in ARRAY_REF of DECL_P case.

Yes, that fixes the same set of sh64-elf regressions that my three alignment 
patches yesterday addressed.
	
-- 
--------------------------
SuperH
2430 Aztec West / Almondsbury / BRISTOL / BS32 4AQ
T:+44 1454 462330

Re: RF[DA]: fix execute/931004-10.c regressions

[Richard Henderson]

On Thu, Jul 25, 2002 at 10:33:30AM +0100, Joern Rennecke wrote:
> + /* ALIGN is the alignment of the MEM itself, while EXPR can describe a
> +    larger underlying object, which might have a stricter alignment; SIZE
> +    is the size of the object, and OFFSET is the offset of the MEM within
> +    that object.  */

Actually, I don't know what SIZE is.  I think it's the size of the
MEM itself.  It's not redundant when the MEM is BLKmode.  That's
what the code in set_mem_attributes seems to do, anyway.

Otherwise ok.


r~

Re: RF[DA]: fix execute/931004-10.c regressions

[Joern Rennecke]

Richard Henderson wrote:
> Actually, I don't know what SIZE is.  I think it's the size of the
> MEM itself.  It's not redundant when the MEM is BLKmode.  That's
> what the code in set_mem_attributes seems to do, anyway.

The debugger said that SIZE was eight, even though the MEM referred
to just two bytes, and that was even so before you patch.  Is that
another bug?
	
-- 
--------------------------
SuperH
2430 Aztec West / Almondsbury / BRISTOL / BS32 4AQ
T:+44 1454 462330

Re: RF[DA]: fix execute/931004-10.c regressions

[Richard Henderson]

On Thu, Jul 25, 2002 at 06:29:23PM +0100, Joern Rennecke wrote:
> The debugger said that SIZE was eight, even though the MEM referred
> to just two bytes, and that was even so before you patch.  Is that
> another bug?

Err, it does?

struct tiny
{
  char c;
  char d;
};

main ()
{
  struct tiny x[3];
  f (3, x[0], x[1], x[2], (long) 123);
  exit(0);
}

(insn 15 14 16 (nil) (set (reg:DI 166)
        (mem/s/j:DI (reg/f:SI 154 virtual-stack-vars) [0 x+0 S8 A64])) -1 (nil)
    (nil))

(insn 23 22 24 (nil) (set (reg:DI 171)
        (zero_extend:DI (mem/s/j:HI (reg/f:SI 161) [0 x+2 S2 A16]))) -1 (nil)
    (nil))

(insn 30 29 31 (nil) (set (reg:SI 177)
        (mem/s/j:SI (reg/f:SI 163) [0 x+4 S4 A32])) -1 (nil)
    (nil))

All of those look correct to me...


r~

Re: RF[DA]: fix execute/931004-10.c regressions

[Joern Rennecke]

Richard Henderson wrote:
> 
> On Thu, Jul 25, 2002 at 06:29:23PM +0100, Joern Rennecke wrote:
> > The debugger said that SIZE was eight, even though the MEM referred
> > to just two bytes, and that was even so before you patch.  Is that
> > another bug?
> 
> Err, it does?

Hmm, looking at the alias.c source it should indeed be the size of
the mem itself.	

> All of those look correct to me...

Yes, today I also see a size of 16 bits where yesterday I saw 64...
I suppose it got also fixed with your patch.

I'll change the comment to:

/* ALIGN and SIZE are the alignment and size of the MEM itself,
   while EXPR can describe a larger underlying object, which might have a
   stricter alignment; OFFSET is the offset of the MEM within that object.  */
	
-- 
--------------------------
SuperH
2430 Aztec West / Almondsbury / BRISTOL / BS32 4AQ
T:+44 1454 462330

Re: RF[DA]: fix execute/931004-10.c regressions

[Richard Kenner]

    Actually, I don't know what SIZE is.  I think it's the size of the
    MEM itself.  It's not redundant when the MEM is BLKmode.

That's correct.