Non-representable subregs of subword pseudoregs

Non-representable subregs of subword pseudoregs

[Eric Botcazou]

Hello,

I've run into a surprising (to me) behavior of the reload pass: it apparently 
doesn't know how to reload non-representable subregs of subword pseudoregs.
For example on SPARC64:

(insn 22 21 23 1 (set (reg:SI 109 [ D.1130 ])
        (subreg:SI (reg:DI 126) 0)) 51 {*movsi_insn} (nil)
    (nil))

leads to

Reloads for insn # 22
Reload 0: reload_in (SI) = (reg:SI 1 %g1)
	GENERAL_REGS, RELOAD_FOR_INPUT (opnum = 1), can't combine
	reload_in_reg: (subreg:SI (reg:DI 1 %g1 [126]) 0)
	reload_reg_rtx: (reg:SI 5 %g5)

(insn 61 21 22 1 (set (reg:SI 5 %g5)
        (reg:SI 1 %g1)) 51 {*movsi_insn} (nil)
    (nil))

(insn 22 61 23 1 (set (reg:SI 4 %g4 [orig:109 D.1130 ] [109])
        (reg:SI 5 %g5)) 51 {*movsi_insn} (nil)
    (nil))

which is of course wrong since the target is big-endian.


A reload is scheduled because find_reload correctly diagnoses that the subreg 
is not representable:

	  while (GET_CODE (operand) == SUBREG)
	    {
	      /* Offset only matters when operand is a REG and
		 it is a hard reg.  This is because it is passed
		 to reg_fits_class_p if it is a REG and all pseudos
		 return 0 from that function.  */
	      if (REG_P (SUBREG_REG (operand))
		  && REGNO (SUBREG_REG (operand)) < FIRST_PSEUDO_REGISTER)
		{
		  if (!subreg_offset_representable_p
			(REGNO (SUBREG_REG (operand)),
			 GET_MODE (SUBREG_REG (operand)),
			 SUBREG_BYTE (operand),
			 GET_MODE (operand)))
		     force_reload = 1;

However, reload_inner_reg_of_subreg returns false for it (for 2 reasons: it's 
an input reload and the function only looks at output reloads, and subregs of 
subword regs are rejected) so push_reload silently turns the invalid subreg 
into the low part:

  /* If IN is a SUBREG of a hard register, make a new REG.  This
     simplifies some of the cases below.  */

  if (in != 0 && GET_CODE (in) == SUBREG && REG_P (SUBREG_REG (in))
      && REGNO (SUBREG_REG (in)) < FIRST_PSEUDO_REGISTER
      && ! dont_remove_subreg)
    in = gen_rtx_REG (GET_MODE (in), subreg_regno (in));


Is this a known limitation of the reload pass or am I missing anything else?
[Jan, I've CCed you because you devised subreg_offset_representable_p.]


The non-representable subreg is created by emit_move_insn_1 when it is invoked 
on pseudo regs and CONCATs with complex modes.  Note that it already has a 
special provision in order not to create non-representable subregs of hard 
regs:

	  /* If this is a complex value with each part being smaller than a
	     word, the usual calling sequence will likely pack the pieces into
	     a single register.  Unfortunately, SUBREG of hard registers only
	     deals in terms of words, so we have a problem converting input
	     arguments to the CONCAT of two registers that is used elsewhere
	     for complex values.  If this is before reload, we can copy it into
	     memory and reload.  FIXME, we should see about using extract and
	     insert on integer registers, but complex short and complex char
	     variables should be rarely used.  */
	  if (GET_MODE_BITSIZE (mode) < 2 * BITS_PER_WORD
	      && (reload_in_progress | reload_completed) == 0)
	    {
	      int packed_dest_p
		= (REG_P (x) && REGNO (x) < FIRST_PSEUDO_REGISTER);
	      int packed_src_p
		= (REG_P (y) && REGNO (y) < FIRST_PSEUDO_REGISTER);

	      if (packed_dest_p || packed_src_p)

but it doesn't apply here because we are dealing with pseudo regs.


How is this supposed to work?  Is it simply illegal to create non-lowpart 
subregs of subword pseudo regs?

Thanks in advance.

-- 
Eric Botcazou

Re: Non-representable subregs of subword pseudoregs

[Jan Hubicka]

> Hello,
> 
> I've run into a surprising (to me) behavior of the reload pass: it apparently 
> doesn't know how to reload non-representable subregs of subword pseudoregs.
> For example on SPARC64:
> 
> (insn 22 21 23 1 (set (reg:SI 109 [ D.1130 ])
>         (subreg:SI (reg:DI 126) 0)) 51 {*movsi_insn} (nil)
>     (nil))
> 
> leads to
> 
> Reloads for insn # 22
> Reload 0: reload_in (SI) = (reg:SI 1 %g1)
> 	GENERAL_REGS, RELOAD_FOR_INPUT (opnum = 1), can't combine
> 	reload_in_reg: (subreg:SI (reg:DI 1 %g1 [126]) 0)
> 	reload_reg_rtx: (reg:SI 5 %g5)
> 
> (insn 61 21 22 1 (set (reg:SI 5 %g5)
>         (reg:SI 1 %g1)) 51 {*movsi_insn} (nil)
>     (nil))
> 
> (insn 22 61 23 1 (set (reg:SI 4 %g4 [orig:109 D.1130 ] [109])
>         (reg:SI 5 %g5)) 51 {*movsi_insn} (nil)
>     (nil))
> 
> which is of course wrong since the target is big-endian.
> 
> 
> A reload is scheduled because find_reload correctly diagnoses that the subreg 
> is not representable:
> 
> 	  while (GET_CODE (operand) == SUBREG)
> 	    {
> 	      /* Offset only matters when operand is a REG and
> 		 it is a hard reg.  This is because it is passed
> 		 to reg_fits_class_p if it is a REG and all pseudos
> 		 return 0 from that function.  */
> 	      if (REG_P (SUBREG_REG (operand))
> 		  && REGNO (SUBREG_REG (operand)) < FIRST_PSEUDO_REGISTER)
> 		{
> 		  if (!subreg_offset_representable_p
> 			(REGNO (SUBREG_REG (operand)),
> 			 GET_MODE (SUBREG_REG (operand)),
> 			 SUBREG_BYTE (operand),
> 			 GET_MODE (operand)))
> 		     force_reload = 1;
> 
> However, reload_inner_reg_of_subreg returns false for it (for 2 reasons: it's 
> an input reload and the function only looks at output reloads, and subregs of 
> subword regs are rejected) so push_reload silently turns the invalid subreg 
> into the low part:
> 
>   /* If IN is a SUBREG of a hard register, make a new REG.  This
>      simplifies some of the cases below.  */
> 
>   if (in != 0 && GET_CODE (in) == SUBREG && REG_P (SUBREG_REG (in))
>       && REGNO (SUBREG_REG (in)) < FIRST_PSEUDO_REGISTER
>       && ! dont_remove_subreg)
>     in = gen_rtx_REG (GET_MODE (in), subreg_regno (in));
> 
> 
> Is this a known limitation of the reload pass or am I missing anything else?
> [Jan, I've CCed you because you devised subreg_offset_representable_p.]
> 
> 
> The non-representable subreg is created by emit_move_insn_1 when it is invoked 
> on pseudo regs and CONCATs with complex modes.  Note that it already has a 
> special provision in order not to create non-representable subregs of hard 
> regs:
> 
> 	  /* If this is a complex value with each part being smaller than a
> 	     word, the usual calling sequence will likely pack the pieces into
> 	     a single register.  Unfortunately, SUBREG of hard registers only
> 	     deals in terms of words, so we have a problem converting input
> 	     arguments to the CONCAT of two registers that is used elsewhere
> 	     for complex values.  If this is before reload, we can copy it into
> 	     memory and reload.  FIXME, we should see about using extract and
> 	     insert on integer registers, but complex short and complex char
> 	     variables should be rarely used.  */
> 	  if (GET_MODE_BITSIZE (mode) < 2 * BITS_PER_WORD
> 	      && (reload_in_progress | reload_completed) == 0)
> 	    {
> 	      int packed_dest_p
> 		= (REG_P (x) && REGNO (x) < FIRST_PSEUDO_REGISTER);
> 	      int packed_src_p
> 		= (REG_P (y) && REGNO (y) < FIRST_PSEUDO_REGISTER);
> 
> 	      if (packed_dest_p || packed_src_p)
> 
> but it doesn't apply here because we are dealing with pseudo regs.
> 
> 
> How is this supposed to work?  Is it simply illegal to create non-lowpart 
> subregs of subword pseudo regs?

I really don't know. We seem to be really chaotic about handing the
subregs here.  I've added the representability bits mostly to make
reload to something instead of leaking the subregs in code to be ignored
later by subreg stripping pass.
The sollution I used for SSE was to avoid these subregs from existence
as far as I can remember.

Honza
> 
> Thanks in advance.
> 
> -- 
> Eric Botcazou

Re: Non-representable subregs of subword pseudoregs

[Richard Sandiford]

Eric Botcazou <ebotcazou@libertysurf.fr> writes:
> I've run into a surprising (to me) behavior of the reload pass: it
> apparently doesn't know how to reload non-representable subregs of
> subword pseudoregs.

ISTR this is the reason why 20020227-1.c is XFAILed for 64-bit targets
(aka. PR 6221, and perhaps others).

Richard

Re: Non-representable subregs of subword pseudoregs

[Eric Botcazou]

> ISTR this is the reason why 20020227-1.c is XFAILed for 64-bit targets
> (aka. PR 6221, and perhaps others).

Definitely, it's the same context (unaligned complex numbers).  Note that I'm 
seeing a similar problem on SPARC 32-bit too.

-- 
Eric Botcazou

[cft] subreg validation patch

[Richard Henderson]

On Sat, Nov 13, 2004 at 10:45:50AM +0100, Eric Botcazou wrote:
> Is it simply illegal to create non-lowpart subregs of subword pseudo regs?

Yes.

I have a rather large patch that attempts to address some of this.
I'd gotten distracted this last week with bitfield fallout, but I
promised Aldy that I'd get back to it.

Could everyone please try the following on their favorite platform
and report back with new failures?


r~



Index: combine.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/combine.c,v
retrieving revision 1.460
diff -c -p -d -r1.460 combine.c
*** combine.c	22 Oct 2004 17:05:03 -0000	1.460
--- combine.c	13 Nov 2004 22:50:29 -0000
*************** recog_for_combine (rtx *pnewpat, rtx ins
*** 9308,9323 ****
     An insn containing that will not be recognized.  */
  
  static rtx
! gen_lowpart_for_combine (enum machine_mode mode, rtx x)
  {
    rtx result;
  
!   if (GET_MODE (x) == mode)
      return x;
  
!   /* Return identity if this is a CONST or symbolic
!      reference.  */
!   if (mode == Pmode
        && (GET_CODE (x) == CONST
  	  || GET_CODE (x) == SYMBOL_REF
  	  || GET_CODE (x) == LABEL_REF))
--- 9308,9325 ----
     An insn containing that will not be recognized.  */
  
  static rtx
! gen_lowpart_for_combine (enum machine_mode omode, rtx x)
  {
+   enum machine_mode imode = GET_MODE (x);
+   unsigned int osize = GET_MODE_SIZE (omode);
+   unsigned int isize = GET_MODE_SIZE (imode);
    rtx result;
  
!   if (omode == imode)
      return x;
  
!   /* Return identity if this is a CONST or symbolic reference.  */
!   if (omode == Pmode
        && (GET_CODE (x) == CONST
  	  || GET_CODE (x) == SYMBOL_REF
  	  || GET_CODE (x) == LABEL_REF))
*************** gen_lowpart_for_combine (enum machine_mo
*** 9325,9337 ****
  
    /* We can only support MODE being wider than a word if X is a
       constant integer or has a mode the same size.  */
! 
!   if (GET_MODE_SIZE (mode) > UNITS_PER_WORD
!       && ! ((GET_MODE (x) == VOIDmode
  	     && (GET_CODE (x) == CONST_INT
  		 || GET_CODE (x) == CONST_DOUBLE))
! 	    || GET_MODE_SIZE (GET_MODE (x)) == GET_MODE_SIZE (mode)))
!     return gen_rtx_CLOBBER (GET_MODE (x), const0_rtx);
  
    /* X might be a paradoxical (subreg (mem)).  In that case, gen_lowpart
       won't know what to do.  So we will strip off the SUBREG here and
--- 9327,9338 ----
  
    /* We can only support MODE being wider than a word if X is a
       constant integer or has a mode the same size.  */
!   if (GET_MODE_SIZE (omode) > UNITS_PER_WORD
!       && ! ((imode == VOIDmode
  	     && (GET_CODE (x) == CONST_INT
  		 || GET_CODE (x) == CONST_DOUBLE))
! 	    || isize == osize))
!     goto fail;
  
    /* X might be a paradoxical (subreg (mem)).  In that case, gen_lowpart
       won't know what to do.  So we will strip off the SUBREG here and
*************** gen_lowpart_for_combine (enum machine_mo
*** 9339,9349 ****
    if (GET_CODE (x) == SUBREG && MEM_P (SUBREG_REG (x)))
      {
        x = SUBREG_REG (x);
!       if (GET_MODE (x) == mode)
  	return x;
      }
  
!   result = gen_lowpart_common (mode, x);
  #ifdef CANNOT_CHANGE_MODE_CLASS
    if (result != 0 && GET_CODE (result) == SUBREG)
      record_subregs_of_mode (result);
--- 9340,9351 ----
    if (GET_CODE (x) == SUBREG && MEM_P (SUBREG_REG (x)))
      {
        x = SUBREG_REG (x);
!       if (GET_MODE (x) == omode)
  	return x;
      }
  
!   result = gen_lowpart_common (omode, x);
! 
  #ifdef CANNOT_CHANGE_MODE_CLASS
    if (result != 0 && GET_CODE (result) == SUBREG)
      record_subregs_of_mode (result);
*************** gen_lowpart_for_combine (enum machine_mo
*** 9359,9391 ****
        /* Refuse to work on a volatile memory ref or one with a mode-dependent
  	 address.  */
        if (MEM_VOLATILE_P (x) || mode_dependent_address_p (XEXP (x, 0)))
! 	return gen_rtx_CLOBBER (GET_MODE (x), const0_rtx);
  
        /* If we want to refer to something bigger than the original memref,
  	 generate a paradoxical subreg instead.  That will force a reload
  	 of the original memref X.  */
!       if (GET_MODE_SIZE (GET_MODE (x)) < GET_MODE_SIZE (mode))
! 	return gen_rtx_SUBREG (mode, x, 0);
  
        if (WORDS_BIG_ENDIAN)
! 	offset = (MAX (GET_MODE_SIZE (GET_MODE (x)), UNITS_PER_WORD)
! 		  - MAX (GET_MODE_SIZE (mode), UNITS_PER_WORD));
  
        if (BYTES_BIG_ENDIAN)
! 	{
! 	  /* Adjust the address so that the address-after-the-data is
! 	     unchanged.  */
! 	  offset -= (MIN (UNITS_PER_WORD, GET_MODE_SIZE (mode))
! 		     - MIN (UNITS_PER_WORD, GET_MODE_SIZE (GET_MODE (x))));
! 	}
  
!       return adjust_address_nv (x, mode, offset);
      }
  
    /* If X is a comparison operator, rewrite it in a new mode.  This
       probably won't match, but may allow further simplifications.  */
    else if (COMPARISON_P (x))
!     return gen_rtx_fmt_ee (GET_CODE (x), mode, XEXP (x, 0), XEXP (x, 1));
  
    /* If we couldn't simplify X any other way, just enclose it in a
       SUBREG.  Normally, this SUBREG won't match, but some patterns may
--- 9361,9388 ----
        /* Refuse to work on a volatile memory ref or one with a mode-dependent
  	 address.  */
        if (MEM_VOLATILE_P (x) || mode_dependent_address_p (XEXP (x, 0)))
! 	goto fail;
  
        /* If we want to refer to something bigger than the original memref,
  	 generate a paradoxical subreg instead.  That will force a reload
  	 of the original memref X.  */
!       if (isize < osize)
! 	return gen_rtx_SUBREG (omode, x, 0);
  
        if (WORDS_BIG_ENDIAN)
! 	offset = MAX (isize, UNITS_PER_WORD) - MAX (osize, UNITS_PER_WORD);
  
+       /* Adjust the address so that the address-after-the-data is unchanged. */
        if (BYTES_BIG_ENDIAN)
! 	offset -= MIN (UNITS_PER_WORD, osize) - MIN (UNITS_PER_WORD, isize);
  
!       return adjust_address_nv (x, omode, offset);
      }
  
    /* If X is a comparison operator, rewrite it in a new mode.  This
       probably won't match, but may allow further simplifications.  */
    else if (COMPARISON_P (x))
!     return gen_rtx_fmt_ee (GET_CODE (x), omode, XEXP (x, 0), XEXP (x, 1));
  
    /* If we couldn't simplify X any other way, just enclose it in a
       SUBREG.  Normally, this SUBREG won't match, but some patterns may
*************** gen_lowpart_for_combine (enum machine_mo
*** 9394,9414 ****
      {
        int offset = 0;
        rtx res;
-       enum machine_mode sub_mode = GET_MODE (x);
  
!       offset = subreg_lowpart_offset (mode, sub_mode);
!       if (sub_mode == VOIDmode)
  	{
! 	  sub_mode = int_mode_for_mode (mode);
! 	  x = gen_lowpart_common (sub_mode, x);
! 	  if (x == 0)
! 	    return gen_rtx_CLOBBER (VOIDmode, const0_rtx);
  	}
!       res = simplify_gen_subreg (mode, x, sub_mode, offset);
        if (res)
  	return res;
-       return gen_rtx_CLOBBER (GET_MODE (x), const0_rtx);
      }
  }
  \f
  /* These routines make binary and unary operations by first seeing if they
--- 9391,9412 ----
      {
        int offset = 0;
        rtx res;
  
!       offset = subreg_lowpart_offset (omode, imode);
!       if (imode == VOIDmode)
  	{
! 	  imode = int_mode_for_mode (omode);
! 	  x = gen_lowpart_common (imode, x);
! 	  if (x == NULL)
! 	    goto fail;
  	}
!       res = simplify_gen_subreg (omode, x, imode, offset);
        if (res)
  	return res;
      }
+ 
+  fail:
+   return gen_rtx_CLOBBER (imode, const0_rtx);
  }
  \f
  /* These routines make binary and unary operations by first seeing if they
Index: cse.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/cse.c,v
retrieving revision 1.320
diff -c -p -d -r1.320 cse.c
*** cse.c	2 Nov 2004 17:59:41 -0000	1.320
--- cse.c	13 Nov 2004 22:50:33 -0000
*************** record_jump_equiv (rtx insn, int taken)
*** 4329,4334 ****
--- 4329,4346 ----
    record_jump_cond (code, mode, op0, op1, reversed_nonequality);
  }
  
+ /* Yet another form of subreg creation.  In this case, we want something in
+    MODE, and we should assume OP has MODE iff it is naturally modeless.  */
+ 
+ static rtx
+ record_jump_cond_subreg (enum machine_mode mode, rtx op)
+ {
+   enum machine_mode op_mode = GET_MODE (op);
+   if (op_mode == mode || op_mode == VOIDmode)
+     return op;
+   return lowpart_subreg (mode, op, op_mode);
+ }
+ 
  /* We know that comparison CODE applied to OP0 and OP1 in MODE is true.
     REVERSED_NONEQUALITY is nonzero if CODE had to be swapped.
     Make any useful entries we can with that information.  Called from
*************** record_jump_cond (enum rtx_code code, en
*** 4353,4363 ****
  	  > GET_MODE_SIZE (GET_MODE (SUBREG_REG (op0)))))
      {
        enum machine_mode inner_mode = GET_MODE (SUBREG_REG (op0));
!       rtx tem = gen_lowpart (inner_mode, op1);
! 
!       record_jump_cond (code, mode, SUBREG_REG (op0),
! 			tem ? tem : gen_rtx_SUBREG (inner_mode, op1, 0),
! 			reversed_nonequality);
      }
  
    if (code == EQ && GET_CODE (op1) == SUBREG
--- 4365,4374 ----
  	  > GET_MODE_SIZE (GET_MODE (SUBREG_REG (op0)))))
      {
        enum machine_mode inner_mode = GET_MODE (SUBREG_REG (op0));
!       rtx tem = record_jump_cond_subreg (inner_mode, op1);
!       if (tem)
! 	record_jump_cond (code, mode, SUBREG_REG (op0), tem,
! 			  reversed_nonequality);
      }
  
    if (code == EQ && GET_CODE (op1) == SUBREG
*************** record_jump_cond (enum rtx_code code, en
*** 4365,4375 ****
  	  > GET_MODE_SIZE (GET_MODE (SUBREG_REG (op1)))))
      {
        enum machine_mode inner_mode = GET_MODE (SUBREG_REG (op1));
!       rtx tem = gen_lowpart (inner_mode, op0);
! 
!       record_jump_cond (code, mode, SUBREG_REG (op1),
! 			tem ? tem : gen_rtx_SUBREG (inner_mode, op0, 0),
! 			reversed_nonequality);
      }
  
    /* Similarly, if this is an NE comparison, and either is a SUBREG
--- 4376,4385 ----
  	  > GET_MODE_SIZE (GET_MODE (SUBREG_REG (op1)))))
      {
        enum machine_mode inner_mode = GET_MODE (SUBREG_REG (op1));
!       rtx tem = record_jump_cond_subreg (inner_mode, op0);
!       if (tem)
! 	record_jump_cond (code, mode, SUBREG_REG (op1), tem,
! 			  reversed_nonequality);
      }
  
    /* Similarly, if this is an NE comparison, and either is a SUBREG
*************** record_jump_cond (enum rtx_code code, en
*** 4385,4395 ****
  	  < GET_MODE_SIZE (GET_MODE (SUBREG_REG (op0)))))
      {
        enum machine_mode inner_mode = GET_MODE (SUBREG_REG (op0));
!       rtx tem = gen_lowpart (inner_mode, op1);
! 
!       record_jump_cond (code, mode, SUBREG_REG (op0),
! 			tem ? tem : gen_rtx_SUBREG (inner_mode, op1, 0),
! 			reversed_nonequality);
      }
  
    if (code == NE && GET_CODE (op1) == SUBREG
--- 4395,4404 ----
  	  < GET_MODE_SIZE (GET_MODE (SUBREG_REG (op0)))))
      {
        enum machine_mode inner_mode = GET_MODE (SUBREG_REG (op0));
!       rtx tem = record_jump_cond_subreg (inner_mode, op1);
!       if (tem)
! 	record_jump_cond (code, mode, SUBREG_REG (op0), tem,
! 			  reversed_nonequality);
      }
  
    if (code == NE && GET_CODE (op1) == SUBREG
*************** record_jump_cond (enum rtx_code code, en
*** 4398,4408 ****
  	  < GET_MODE_SIZE (GET_MODE (SUBREG_REG (op1)))))
      {
        enum machine_mode inner_mode = GET_MODE (SUBREG_REG (op1));
!       rtx tem = gen_lowpart (inner_mode, op0);
! 
!       record_jump_cond (code, mode, SUBREG_REG (op1),
! 			tem ? tem : gen_rtx_SUBREG (inner_mode, op0, 0),
! 			reversed_nonequality);
      }
  
    /* Hash both operands.  */
--- 4407,4416 ----
  	  < GET_MODE_SIZE (GET_MODE (SUBREG_REG (op1)))))
      {
        enum machine_mode inner_mode = GET_MODE (SUBREG_REG (op1));
!       rtx tem = record_jump_cond_subreg (inner_mode, op0);
!       if (tem)
! 	record_jump_cond (code, mode, SUBREG_REG (op1), tem,
! 			  reversed_nonequality);
      }
  
    /* Hash both operands.  */
Index: emit-rtl.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/emit-rtl.c,v
retrieving revision 1.423
diff -c -p -d -r1.423 emit-rtl.c
*** emit-rtl.c	9 Nov 2004 17:46:06 -0000	1.423
--- emit-rtl.c	13 Nov 2004 22:50:34 -0000
*************** gen_const_mem (enum machine_mode mode, r
*** 607,626 ****
    return mem;
  }
  
  rtx
  gen_rtx_SUBREG (enum machine_mode mode, rtx reg, int offset)
  {
!   /* This is the most common failure type.
!      Catch it early so we can see who does it.  */
!   gcc_assert (!(offset % GET_MODE_SIZE (mode)));
! 
!   /* This check isn't usable right now because combine will
!      throw arbitrary crap like a CALL into a SUBREG in
!      gen_lowpart_for_combine so we must just eat it.  */
! #if 0
!   /* Check for this too.  */
!   gcc_assert (offset < GET_MODE_SIZE (GET_MODE (reg)));
! #endif
    return gen_rtx_raw_SUBREG (mode, reg, offset);
  }
  
--- 607,688 ----
    return mem;
  }
  
+ /* We want to create (subreg:OMODE (obj:IMODE) OFFSET).  Return true if
+    this construct would be valid, and false otherwise.  */
+ 
+ bool
+ validate_subreg (enum machine_mode omode, enum machine_mode imode,
+ 		 rtx reg, unsigned int offset)
+ {
+   unsigned int isize = GET_MODE_SIZE (imode);
+   unsigned int osize = GET_MODE_SIZE (omode);
+ 
+   /* All subregs must be aligned.  */
+   if (offset % osize != 0)
+     return false;
+ 
+   /* The subreg offset cannot be outside the inner object.  */
+   if (offset >= isize)
+     return false;
+ 
+   /* ??? This should not be here.  Temporarily continue to allow word_mode
+      subregs of anything.  The most common offender is (subreg:SI (reg:DF)).
+      Generally, backends are doing something sketchy but it'll take time to
+      fix them all.  */
+   if (omode == word_mode)
+     ;
+   /* Allow component subregs of complex and vector.  Though given the below
+      extraction rules, it's not always clear what that means.  */
+   else if ((COMPLEX_MODE_P (imode) || VECTOR_MODE_P (imode))
+ 	   && GET_MODE_INNER (imode) == omode)
+     ;
+   /* ??? x86 sse code makes heavy use of *paradoxical* vector subregs,
+      i.e. (subreg:V4SF (reg:SF) 0).  This surely isn't the cleanest way to
+      represent this.  It's questionable if this ought to be represented at
+      all -- why can't this all be hidden in post-reload splitters that make
+      arbitrarily mode changes to the registers themselves.  */
+   else if (VECTOR_MODE_P (omode) && GET_MODE_INNER (omode) == imode)
+     ;
+   /* Subregs involving floating point modes are not allowed to
+      change size.  Therefore (subreg:DI (reg:DF) 0) is fine, but
+      (subreg:SI (reg:DF) 0) isn't.  */
+   else if (FLOAT_MODE_P (imode) || FLOAT_MODE_P (omode))
+     {
+       if (isize != osize)
+ 	return false;
+     }
+ 
+   /* Paradoxical subregs must have offset zero.  */
+   if (osize > isize)
+     return offset == 0;
+ 
+   /* This is a normal subreg.  Verify that the offset is representable.  */
+ 
+   /* For hard registers, we already have all of these rules collected in
+      subreg_offset_representable_p.  */
+   if (reg && REG_P (reg) && HARD_REGISTER_P (reg))
+     return subreg_offset_representable_p (REGNO (reg), imode, offset, omode);
+ 
+   /* For pseudo registers, we want most of the same checks.  Namely:
+      If the register no larger than a word, the subreg must be lowpart.
+      If the register is larger than a word, the subreg must be the lowpart
+      of a subword.  A subreg does *not* perform arbitrary bit extraction.
+      Given that we've already checked mode/offset alignment, we only have
+      to check subword subregs here.  */
+   if (osize < UNITS_PER_WORD)
+     {
+       enum machine_mode wmode = isize > UNITS_PER_WORD ? word_mode : imode;
+       unsigned int low_off = subreg_lowpart_offset (omode, wmode);
+       if (offset % UNITS_PER_WORD != low_off)
+ 	return false;
+     }
+   return true;
+ }
+ 
  rtx
  gen_rtx_SUBREG (enum machine_mode mode, rtx reg, int offset)
  {
!   gcc_assert (validate_subreg (mode, GET_MODE (reg), reg, offset));
    return gen_rtx_raw_SUBREG (mode, reg, offset);
  }
  
Index: expmed.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/expmed.c,v
retrieving revision 1.203
diff -c -p -d -r1.203 expmed.c
*** expmed.c	8 Nov 2004 19:18:16 -0000	1.203
--- expmed.c	13 Nov 2004 22:50:35 -0000
*************** extract_bit_field (rtx str_rtx, unsigned
*** 1233,1254 ****
      {
        if (mode1 != GET_MODE (op0))
  	{
! 	  if (GET_CODE (op0) == SUBREG)
  	    {
! 	      if (GET_MODE (SUBREG_REG (op0)) == mode1
! 		  || GET_MODE_CLASS (mode1) == MODE_INT
! 		  || GET_MODE_CLASS (mode1) == MODE_PARTIAL_INT)
! 		op0 = SUBREG_REG (op0);
! 	      else
! 		/* Else we've got some float mode source being extracted into
! 		   a different float mode destination -- this combination of
! 		   subregs results in Severe Tire Damage.  */
  		goto no_subreg_mode_swap;
  	    }
- 	  if (REG_P (op0))
- 	    op0 = gen_rtx_SUBREG (mode1, op0, byte_offset);
- 	  else
- 	    op0 = adjust_address (op0, mode1, offset);
  	}
        if (mode1 != mode)
  	return convert_to_mode (tmode, op0, unsignedp);
--- 1233,1248 ----
      {
        if (mode1 != GET_MODE (op0))
  	{
! 	  if (MEM_P (op0))
! 	    op0 = adjust_address (op0, mode1, offset);
! 	  else
  	    {
! 	      rtx sub = simplify_gen_subreg (mode1, op0, GET_MODE (op0),
! 					     byte_offset);
! 	      if (sub == NULL)
  		goto no_subreg_mode_swap;
+ 	      op0 = sub;
  	    }
  	}
        if (mode1 != mode)
  	return convert_to_mode (tmode, op0, unsignedp);
*************** extract_bit_field (rtx str_rtx, unsigned
*** 1628,1646 ****
      return spec_target;
    if (GET_MODE (target) != tmode && GET_MODE (target) != mode)
      {
!       /* If the target mode is floating-point, first convert to the
! 	 integer mode of that size and then access it as a floating-point
! 	 value via a SUBREG.  */
!       if (GET_MODE_CLASS (tmode) != MODE_INT
! 	  && GET_MODE_CLASS (tmode) != MODE_PARTIAL_INT)
  	{
  	  target = convert_to_mode (mode_for_size (GET_MODE_BITSIZE (tmode),
  						   MODE_INT, 0),
  				    target, unsignedp);
  	  return gen_lowpart (tmode, target);
  	}
!       else
! 	return convert_to_mode (tmode, target, unsignedp);
      }
    return target;
  }
--- 1622,1662 ----
      return spec_target;
    if (GET_MODE (target) != tmode && GET_MODE (target) != mode)
      {
!       /* If the target mode is complex, then extract the two scalar elements
! 	 from the value now.  Creating (subreg:SC (reg:DI) 0), as we would do
! 	 with the clause below, will cause gen_realpart or gen_imagpart to
! 	 fail, since those functions must return lvalues.  */
!       if (COMPLEX_MODE_P (tmode))
  	{
+ 	  rtx realpart, imagpart;
+ 	  enum machine_mode itmode = GET_MODE_INNER (tmode);
+ 
  	  target = convert_to_mode (mode_for_size (GET_MODE_BITSIZE (tmode),
  						   MODE_INT, 0),
  				    target, unsignedp);
+ 
+ 	  realpart = extract_bit_field (target, GET_MODE_BITSIZE (itmode), 0,
+ 					unsignedp, NULL, itmode, itmode);
+ 	  imagpart = extract_bit_field (target, GET_MODE_BITSIZE (itmode),
+ 					GET_MODE_BITSIZE (itmode), unsignedp,
+ 					NULL, itmode, itmode);
+ 
+ 	  return gen_rtx_CONCAT (tmode, realpart, imagpart);
+ 	}
+ 
+       /* If the target mode is not a scalar integral, first convert to the
+ 	 integer mode of that size and then access it as a floating-point
+ 	 value via a SUBREG.  */
+       if (!SCALAR_INT_MODE_P (tmode))
+ 	{
+ 	  enum machine_mode smode
+ 	    = mode_for_size (GET_MODE_BITSIZE (tmode), MODE_INT, 0);
+ 	  target = convert_to_mode (smode, target, unsignedp);
+ 	  target = force_reg (smode, target);
  	  return gen_lowpart (tmode, target);
  	}
! 
!       return convert_to_mode (tmode, target, unsignedp);
      }
    return target;
  }
Index: expr.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/expr.c,v
retrieving revision 1.739
diff -c -p -d -r1.739 expr.c
*** expr.c	12 Nov 2004 06:59:47 -0000	1.739
--- expr.c	13 Nov 2004 22:50:37 -0000
*************** emit_move_insn_1 (rtx x, rtx y)
*** 2578,2584 ****
  {
    enum machine_mode mode = GET_MODE (x);
    enum machine_mode submode;
-   enum mode_class class = GET_MODE_CLASS (mode);
  
    gcc_assert ((unsigned int) mode < (unsigned int) MAX_MACHINE_MODE);
  
--- 2578,2583 ----
*************** emit_move_insn_1 (rtx x, rtx y)
*** 2587,2606 ****
        emit_insn (GEN_FCN (mov_optab->handlers[(int) mode].insn_code) (x, y));
  
    /* Expand complex moves by moving real part and imag part, if possible.  */
!   else if ((class == MODE_COMPLEX_FLOAT || class == MODE_COMPLEX_INT)
  	   && BLKmode != (submode = GET_MODE_INNER (mode))
  	   && (mov_optab->handlers[(int) submode].insn_code
  	       != CODE_FOR_nothing))
      {
        /* Don't split destination if it is a stack push.  */
!       int stack = push_operand (x, GET_MODE (x));
  
  #ifdef PUSH_ROUNDING
        /* In case we output to the stack, but the size is smaller than the
  	 machine can push exactly, we need to use move instructions.  */
!       if (stack
! 	  && (PUSH_ROUNDING (GET_MODE_SIZE (submode))
! 	      != GET_MODE_SIZE (submode)))
  	{
  	  rtx temp;
  	  HOST_WIDE_INT offset1, offset2;
--- 2586,2606 ----
        emit_insn (GEN_FCN (mov_optab->handlers[(int) mode].insn_code) (x, y));
  
    /* Expand complex moves by moving real part and imag part, if possible.  */
!   else if (COMPLEX_MODE_P (mode)
  	   && BLKmode != (submode = GET_MODE_INNER (mode))
  	   && (mov_optab->handlers[(int) submode].insn_code
  	       != CODE_FOR_nothing))
      {
+       unsigned int modesize = GET_MODE_SIZE (mode);
+       unsigned int submodesize = GET_MODE_SIZE (submode);
+ 
        /* Don't split destination if it is a stack push.  */
!       int stack = push_operand (x, mode);
  
  #ifdef PUSH_ROUNDING
        /* In case we output to the stack, but the size is smaller than the
  	 machine can push exactly, we need to use move instructions.  */
!       if (stack && PUSH_ROUNDING (submodesize) != submodesize)
  	{
  	  rtx temp;
  	  HOST_WIDE_INT offset1, offset2;
*************** emit_move_insn_1 (rtx x, rtx y)
*** 2614,2622 ****
  			       add_optab,
  #endif
  			       stack_pointer_rtx,
! 			       GEN_INT
! 				 (PUSH_ROUNDING
! 				  (GET_MODE_SIZE (GET_MODE (x)))),
  			       stack_pointer_rtx, 0, OPTAB_LIB_WIDEN);
  
  	  if (temp != stack_pointer_rtx)
--- 2614,2620 ----
  			       add_optab,
  #endif
  			       stack_pointer_rtx,
! 			       GEN_INT (PUSH_ROUNDING (modesize)),
  			       stack_pointer_rtx, 0, OPTAB_LIB_WIDEN);
  
  	  if (temp != stack_pointer_rtx)
*************** emit_move_insn_1 (rtx x, rtx y)
*** 2624,2634 ****
  
  #ifdef STACK_GROWS_DOWNWARD
  	  offset1 = 0;
! 	  offset2 = GET_MODE_SIZE (submode);
  #else
! 	  offset1 = -PUSH_ROUNDING (GET_MODE_SIZE (GET_MODE (x)));
! 	  offset2 = (-PUSH_ROUNDING (GET_MODE_SIZE (GET_MODE (x)))
! 		     + GET_MODE_SIZE (submode));
  #endif
  
  	  emit_move_insn (change_address (x, submode,
--- 2622,2631 ----
  
  #ifdef STACK_GROWS_DOWNWARD
  	  offset1 = 0;
! 	  offset2 = submodesize;
  #else
! 	  offset1 = -PUSH_ROUNDING (modesize);
! 	  offset2 = -PUSH_ROUNDING (modesize) + submodesize;
  #endif
  
  	  emit_move_insn (change_address (x, submode,
*************** emit_move_insn_1 (rtx x, rtx y)
*** 2679,2720 ****
  	     memory and reload.  FIXME, we should see about using extract and
  	     insert on integer registers, but complex short and complex char
  	     variables should be rarely used.  */
! 	  if (GET_MODE_BITSIZE (mode) < 2 * BITS_PER_WORD
! 	      && (reload_in_progress | reload_completed) == 0)
  	    {
! 	      int packed_dest_p
! 		= (REG_P (x) && REGNO (x) < FIRST_PSEUDO_REGISTER);
! 	      int packed_src_p
! 		= (REG_P (y) && REGNO (y) < FIRST_PSEUDO_REGISTER);
! 
! 	      if (packed_dest_p || packed_src_p)
  		{
! 		  enum mode_class reg_class = ((class == MODE_COMPLEX_FLOAT)
! 					       ? MODE_FLOAT : MODE_INT);
! 
  		  enum machine_mode reg_mode
! 		    = mode_for_size (GET_MODE_BITSIZE (mode), reg_class, 1);
! 
! 		  if (reg_mode != BLKmode)
! 		    {
! 		      rtx mem = assign_stack_temp (reg_mode,
! 						   GET_MODE_SIZE (mode), 0);
! 		      rtx cmem = adjust_address (mem, mode, 0);
  
! 		      if (packed_dest_p)
! 			{
! 			  rtx sreg = gen_rtx_SUBREG (reg_mode, x, 0);
  
! 			  emit_move_insn_1 (cmem, y);
! 			  return emit_move_insn_1 (sreg, mem);
! 			}
! 		      else
! 			{
! 			  rtx sreg = gen_rtx_SUBREG (reg_mode, y, 0);
  
! 			  emit_move_insn_1 (mem, sreg);
! 			  return emit_move_insn_1 (x, cmem);
! 			}
  		    }
  		}
  	    }
--- 2676,2707 ----
  	     memory and reload.  FIXME, we should see about using extract and
  	     insert on integer registers, but complex short and complex char
  	     variables should be rarely used.  */
! 	  if ((reload_in_progress | reload_completed) == 0
! 	      && (!validate_subreg (submode, mode, NULL, submodesize)
! 		  || !validate_subreg (submode, mode, NULL, 0)))
  	    {
! 	      if (REG_P (x) || REG_P (y))
  		{
! 		  rtx mem, cmem;
  		  enum machine_mode reg_mode
! 		    = mode_for_size (GET_MODE_BITSIZE (mode), MODE_INT, 1);
  
! 		  gcc_assert (reg_mode != BLKmode);
  
! 		  mem = assign_stack_temp (reg_mode, modesize, 0);
! 		  cmem = adjust_address (mem, mode, 0);
  
! 		  if (REG_P (x))
! 		    {
! 		      rtx sreg = gen_rtx_SUBREG (reg_mode, x, 0);
! 		      emit_move_insn_1 (cmem, y);
! 		      return emit_move_insn_1 (sreg, mem);
! 		    }
! 		  else
! 		    {
! 		      rtx sreg = gen_rtx_SUBREG (reg_mode, y, 0);
! 		      emit_move_insn_1 (mem, sreg);
! 		      return emit_move_insn_1 (x, cmem);
  		    }
  		}
  	    }
Index: rtl.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/rtl.h,v
retrieving revision 1.522
diff -c -p -d -r1.522 rtl.h
*** rtl.h	11 Nov 2004 23:10:10 -0000	1.522
--- rtl.h	13 Nov 2004 22:50:37 -0000
*************** extern rtx emit_copy_of_insn_after (rtx,
*** 1372,1378 ****
  extern void set_reg_attrs_from_mem (rtx, rtx);
  extern void set_mem_attrs_from_reg (rtx, rtx);
  extern void set_reg_attrs_for_parm (rtx, rtx);
- extern void set_reg_pointer_align (rtx, unsigned int);
  extern int mem_expr_equal_p (tree, tree);
  
  /* In rtl.c */
--- 1372,1377 ----
*************** extern rtx delete_insn_and_edges (rtx);
*** 1989,1994 ****
--- 1988,1995 ----
  extern void delete_insn_chain_and_edges (rtx, rtx);
  extern rtx gen_lowpart_SUBREG (enum machine_mode, rtx);
  extern rtx gen_const_mem (enum machine_mode, rtx);
+ extern bool validate_subreg (enum machine_mode, enum machine_mode,
+ 			     rtx, unsigned int);
  
  /* In combine.c */
  extern int combine_instructions (rtx, unsigned int);
Index: simplify-rtx.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/simplify-rtx.c,v
retrieving revision 1.207
diff -c -p -d -r1.207 simplify-rtx.c
*** simplify-rtx.c	28 Oct 2004 12:47:21 -0000	1.207
--- simplify-rtx.c	13 Nov 2004 22:50:38 -0000
*************** simplify_subreg (enum machine_mode outer
*** 3641,3652 ****
  	}
  
        /* Recurse for further possible simplifications.  */
!       newx = simplify_subreg (outermode, SUBREG_REG (op),
! 			     GET_MODE (SUBREG_REG (op)),
! 			     final_offset);
        if (newx)
  	return newx;
!       return gen_rtx_SUBREG (outermode, SUBREG_REG (op), final_offset);
      }
  
    /* SUBREG of a hard register => just change the register number
--- 3641,3654 ----
  	}
  
        /* Recurse for further possible simplifications.  */
!       newx = simplify_subreg (outermode, SUBREG_REG (op), innermostmode,
! 			      final_offset);
        if (newx)
  	return newx;
!       if (validate_subreg (outermode, innermostmode,
! 			   SUBREG_REG (op), final_offset))
!         return gen_rtx_SUBREG (outermode, SUBREG_REG (op), final_offset);
!       return NULL_RTX;
      }
  
    /* SUBREG of a hard register => just change the register number
*************** simplify_subreg (enum machine_mode outer
*** 3723,3731 ****
        res = simplify_subreg (outermode, part, GET_MODE (part), final_offset);
        if (res)
  	return res;
!       /* We can at least simplify it by referring directly to the
! 	 relevant part.  */
!       return gen_rtx_SUBREG (outermode, part, final_offset);
      }
  
    /* Optimize SUBREG truncations of zero and sign extended values.  */
--- 3725,3733 ----
        res = simplify_subreg (outermode, part, GET_MODE (part), final_offset);
        if (res)
  	return res;
!       if (validate_subreg (outermode, GET_MODE (part), part, final_offset))
!         return gen_rtx_SUBREG (outermode, part, final_offset);
!       return NULL_RTX;
      }
  
    /* Optimize SUBREG truncations of zero and sign extended values.  */
*************** simplify_gen_subreg (enum machine_mode o
*** 3773,3789 ****
  		     enum machine_mode innermode, unsigned int byte)
  {
    rtx newx;
-   /* Little bit of sanity checking.  */
-   gcc_assert (innermode != VOIDmode);
-   gcc_assert (outermode != VOIDmode);
-   gcc_assert (innermode != BLKmode);
-   gcc_assert (outermode != BLKmode);
- 
-   gcc_assert (GET_MODE (op) == innermode
- 	      || GET_MODE (op) == VOIDmode);
- 
-   gcc_assert ((byte % GET_MODE_SIZE (outermode)) == 0);
-   gcc_assert (byte < GET_MODE_SIZE (innermode));
  
    newx = simplify_subreg (outermode, op, innermode, byte);
    if (newx)
--- 3775,3780 ----
*************** simplify_gen_subreg (enum machine_mode o
*** 3793,3800 ****
        || (REG_P (op) && REGNO (op) < FIRST_PSEUDO_REGISTER))
      return NULL_RTX;
  
!   return gen_rtx_SUBREG (outermode, op, byte);
  }
  /* Simplify X, an rtx expression.
  
     Return the simplified expression or NULL if no simplifications
--- 3784,3795 ----
        || (REG_P (op) && REGNO (op) < FIRST_PSEUDO_REGISTER))
      return NULL_RTX;
  
!   if (validate_subreg (outermode, innermode, op, byte))
!     return gen_rtx_SUBREG (outermode, op, byte);
! 
!   return NULL_RTX;
  }
+ 
  /* Simplify X, an rtx expression.
  
     Return the simplified expression or NULL if no simplifications

Re: [cft] subreg validation patch

[Eric Botcazou]

> Could everyone please try the following on their favorite platform
> and report back with new failures?

Problematic on the SPARC because of

> +   /* Subregs involving floating point modes are not allowed to
> +      change size.  Therefore (subreg:DI (reg:DF) 0) is fine, but
> +      (subreg:SI (reg:DF) 0) isn't.  */
> +   else if (FLOAT_MODE_P (imode) || FLOAT_MODE_P (omode))
> +     {
> +       if (isize != osize)
> + 	return false;
> +     }

which breaks all the DFmode->SFmode splitters.  Should we now create a double 
subreg in this case?

-- 
Eric Botcazou

Re: [cft] subreg validation patch

[Richard Henderson]

On Sun, Nov 14, 2004 at 12:48:05AM +0100, Eric Botcazou wrote:
> which breaks all the DFmode->SFmode splitters.  Should we now create a double 
> subreg in this case?

Why are you splitting these before reload?  And post-reload these
shouldn't create subregs, just new hard regs.


r~

Re: [cft] subreg validation patch

[Eric Botcazou]

> Why are you splitting these before reload?

These are post-reload splitters.

> And post-reload these shouldn't create subregs, just new hard regs.

As far as I can see, almost all splitters in sparc.md use gen_lowpart and 
gen_highpart, disregarding their post-reloadness.  I'll fix them tomorrow.

-- 
Eric Botcazou

Re: [cft] subreg validation patch

[Richard Henderson]

On Sun, Nov 14, 2004 at 01:07:34AM +0100, Eric Botcazou wrote:
> As far as I can see, almost all splitters in sparc.md use gen_lowpart and 
> gen_highpart, disregarding their post-reloadness.  I'll fix them tomorrow.

Except that I would think that gen_lowpart/highpart should be creating
a new hard register rtx when given an input hard register rtx.

I'll look into it later tonight.


r~

Re: [cft] subreg validation patch

[Richard Henderson]

On Sun, Nov 14, 2004 at 12:48:05AM +0100, Eric Botcazou wrote:
> Problematic on the SPARC because of
...
> which breaks all the DFmode->SFmode splitters.

Did you just guess this would be a problem, or did you observe it?

The sparc that I'm using is awfully slow, but it has successfully
built libgcc during stage1.  I would have assumed that if *all* 
DFmode splitters were affected, that I wouldn't have gotten this far.



r~

Re: [cft] subreg validation patch

[Geert Bosch]

On Nov 13, 2004, at 23:41, Richard Henderson wrote:
> The sparc that I'm using is awfully slow, but it has successfully
> built libgcc during stage1.  I would have assumed that if *all*
> DFmode splitters were affected, that I wouldn't have gotten this far.

Are you using a 32-bit SPARC by any chance? I believe Eric's
comments were with regards to a 64-bit target.

   -Geert

Re: [cft] subreg validation patch

[Richard Henderson]

On Sat, Nov 13, 2004 at 08:41:13PM -0800, Richard Henderson wrote:
> On Sun, Nov 14, 2004 at 12:48:05AM +0100, Eric Botcazou wrote:
> > Problematic on the SPARC because of
> ...
> > which breaks all the DFmode->SFmode splitters.
> 
> Did you just guess this would be a problem, or did you observe it?

Saw it myself... finally.  Got a fix for it too.  Nicely irritating
in that we didn't actually *want* a subreg, we just created it so 
that we could pass it to some subroutine, when we should have been
using subreg_regno_offset in the first place.


r~

Re: [cft] subreg validation patch

[Eric Botcazou]

> Saw it myself... finally.  Got a fix for it too.  Nicely irritating
> in that we didn't actually *want* a subreg, we just created it so
> that we could pass it to some subroutine, when we should have been
> using subreg_regno_offset in the first place.

Thanks.  I spotted the problem with Jakub's struct-layout testsuite.  Jakub 
may have advertised it as a compatibility testsuite, but it is extremely 
useful in stressing the middle-end and the back-end (as of today, the only 
failures on the SPARC are due to the subreg representability problem).

-- 
Eric Botcazou

Re: [cft] subreg validation patch

[Ulrich Weigand]

Richard Henderson wrote:

>Could everyone please try the following on their favorite platform
>and report back with new failures?

I've tried your patch on s390-ibm-linux and s390x-ibm-linux.  On both
platforms, bootstrap fails when building stage2, because of a bug in
regmove_optimize.  The code there tries to convert a paradoxical
subreg move into a move with the opposite subreg on the other side,
but completely messes up the subreg offset.  The following patch
appears to fix that problem:

Index: gcc/regmove.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/regmove.c,v
retrieving revision 1.164
diff -c -p -r1.164 regmove.c
*** gcc/regmove.c	9 Oct 2004 19:19:24 -0000	1.164
--- gcc/regmove.c	14 Nov 2004 02:56:01 -0000
*************** regmove_optimize (rtx f, int nregs, FILE
*** 1150,1159 ****
  		  && GET_MODE_SIZE (GET_MODE (dst))
  		     >= GET_MODE_SIZE (GET_MODE (SUBREG_REG (dst))))
  		{
- 		  src_subreg
- 		    = gen_rtx_SUBREG (GET_MODE (SUBREG_REG (dst)),
- 				      src, SUBREG_BYTE (dst));
  		  dst = SUBREG_REG (dst);
  		}
  	      if (!REG_P (dst)
  		  || REGNO (dst) < FIRST_PSEUDO_REGISTER)
--- 1150,1157 ----
  		  && GET_MODE_SIZE (GET_MODE (dst))
  		     >= GET_MODE_SIZE (GET_MODE (SUBREG_REG (dst))))
  		{
  		  dst = SUBREG_REG (dst);
+ 		  src_subreg = gen_lowpart (GET_MODE (dst), src);
  		}
  	      if (!REG_P (dst)
  		  || REGNO (dst) < FIRST_PSEUDO_REGISTER)


With that patch, bootstrap and regtest complete on s390-ibm-linux
without regressions.  On s390x, however, I get a couple of 
regressions:
> FAIL: gcc.c-torture/execute/20040709-1.c compilation,  -O1
> UNRESOLVED: gcc.c-torture/execute/20040709-1.c execution,  -O1
> FAIL: gcc.c-torture/execute/20040709-1.c compilation,  -O2
> UNRESOLVED: gcc.c-torture/execute/20040709-1.c execution,  -O2
> FAIL: gcc.c-torture/execute/930930-2.c compilation,  -O1
> UNRESOLVED: gcc.c-torture/execute/930930-2.c execution,  -O1
> FAIL: gcc.c-torture/execute/930930-2.c compilation,  -O2
> UNRESOLVED: gcc.c-torture/execute/930930-2.c execution,  -O2
> FAIL: gcc.c-torture/execute/930930-2.c compilation,  -O3 -fomit-frame-pointer
> UNRESOLVED: gcc.c-torture/execute/930930-2.c execution,  -O3 -fomit-frame-pointer
> FAIL: gcc.c-torture/execute/930930-2.c compilation,  -O3 -g
> UNRESOLVED: gcc.c-torture/execute/930930-2.c execution,  -O3 -g
> FAIL: gcc.c-torture/execute/930930-2.c compilation,  -Os
> UNRESOLVED: gcc.c-torture/execute/930930-2.c execution,  -Os
> FAIL: gcc.c-torture/execute/stdarg-3.c compilation,  -O1
> UNRESOLVED: gcc.c-torture/execute/stdarg-3.c execution,  -O1
> FAIL: gcc.c-torture/execute/stdarg-3.c compilation,  -O2
> UNRESOLVED: gcc.c-torture/execute/stdarg-3.c execution,  -O2
> FAIL: gcc.c-torture/execute/stdarg-3.c compilation,  -O3 -fomit-frame-pointer
> UNRESOLVED: gcc.c-torture/execute/stdarg-3.c execution,  -O3 -fomit-frame-pointer
> FAIL: gcc.c-torture/execute/stdarg-3.c compilation,  -O3 -fomit-frame-pointer -funroll-loops
> UNRESOLVED: gcc.c-torture/execute/stdarg-3.c execution,  -O3 -fomit-frame-pointer -funroll-loops
> FAIL: gcc.c-torture/execute/stdarg-3.c compilation,  -O3 -fomit-frame-pointer -funroll-all-loops -finline-functions
> UNRESOLVED: gcc.c-torture/execute/stdarg-3.c execution,  -O3 -fomit-frame-pointer -funroll-all-loops -finline-functions
> FAIL: gcc.c-torture/execute/stdarg-3.c compilation,  -O3 -g
> UNRESOLVED: gcc.c-torture/execute/stdarg-3.c execution,  -O3 -g
> FAIL: gcc.c-torture/execute/stdarg-3.c compilation,  -Os
> UNRESOLVED: gcc.c-torture/execute/stdarg-3.c execution,  -Os

These all have the same reason: this piece of code in store_bit_field

  if (bitpos == 0
      && bitsize == GET_MODE_BITSIZE (fieldmode)
      && (!MEM_P (op0)
          ? ((GET_MODE_SIZE (fieldmode) >= UNITS_PER_WORD
             || GET_MODE_SIZE (GET_MODE (op0)) == GET_MODE_SIZE (fieldmode))
             && byte_offset % GET_MODE_SIZE (fieldmode) == 0)
          : (! SLOW_UNALIGNED_ACCESS (fieldmode, MEM_ALIGN (op0))
             || (offset * BITS_PER_UNIT % bitsize == 0
                 && MEM_ALIGN (op0) % GET_MODE_BITSIZE (fieldmode) == 0))))
    {
      if (GET_MODE (op0) != fieldmode)
        {
          if (GET_CODE (op0) == SUBREG)
            {
              /* Else we've got some float mode source being extracted
                 into a different float mode destination -- this
                 combination of subregs results in Severe Tire
                 Damage.  */
              gcc_assert (GET_MODE (SUBREG_REG (op0)) == fieldmode
                          || GET_MODE_CLASS (fieldmode) == MODE_INT
                          || GET_MODE_CLASS (fieldmode) == MODE_PARTIAL_INT);
              op0 = SUBREG_REG (op0);
            }
          if (REG_P (op0))
            op0 = gen_rtx_SUBREG (fieldmode, op0, byte_offset);
          else
            op0 = adjust_address (op0, fieldmode, offset);
        }
      emit_move_insn (op0, value);
      return value;
    }

calls gen_rtx_SUBREG to form a DFmode subreg of one of the subwords of
a TImode register.

I'm not sure how to fix that, not least because I still don't understand
why this subreg is now supposed to be invalid in the first place ...


One other question about your patch:

+   /* For hard registers, we already have all of these rules collected in
+      subreg_offset_representable_p.  */
+   if (reg && REG_P (reg) && HARD_REGISTER_P (reg))
+     return subreg_offset_representable_p (REGNO (reg), imode, offset, omode);

This check is not quite the same as the check done in simplify_subreg.
In particular, you neither check for REG_CANNOT_CHANGE_MODE_P, nor is
there the special checks for frame-related registers.  While I'm not
quite sure I fully understand those checks, shouldn't the rules be
the same here as in simplify_subreg?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  Linux on zSeries Development
  Ulrich.Weigand@de.ibm.com

Re: [cft] subreg validation patch

[Richard Henderson]

On Sun, Nov 14, 2004 at 04:03:39PM +0100, Ulrich Weigand wrote:
> Richard Henderson wrote:
> 
> >Could everyone please try the following on their favorite platform
> >and report back with new failures?
> 
> I've tried your patch on s390-ibm-linux and s390x-ibm-linux.  On both
> platforms, bootstrap fails when building stage2, because of a bug in
> regmove_optimize.  The code there tries to convert a paradoxical
> subreg move into a move with the opposite subreg on the other side,
> but completely messes up the subreg offset.  The following patch
> appears to fix that problem:

Indeed.  This should be ok to check in by itself.

> ... calls gen_rtx_SUBREG to form a DFmode subreg of one of the subwords of
> a TImode register.
> 
> I'm not sure how to fix that, not least because I still don't understand
> why this subreg is now supposed to be invalid in the first place ...

According to the docs, it always was invalid.  I guess I'll put an
exception in there for this, as with the SI->DF exception that I
have already added.

> This check is not quite the same as the check done in simplify_subreg.
> In particular, you neither check for REG_CANNOT_CHANGE_MODE_P, nor is
> there the special checks for frame-related registers.  While I'm not
> quite sure I fully understand those checks, shouldn't the rules be
> the same here as in simplify_subreg?

Yes.


r~

Re: [cft] subreg validation patch

[Ulrich Weigand]

Richard Henderson <rth@redhat.com> wrote on 11/15/2004 09:03:19 PM:

> On Sun, Nov 14, 2004 at 04:03:39PM +0100, Ulrich Weigand wrote:
> > I've tried your patch on s390-ibm-linux and s390x-ibm-linux.  On both
> > platforms, bootstrap fails when building stage2, because of a bug in
> > regmove_optimize.  The code there tries to convert a paradoxical
> > subreg move into a move with the opposite subreg on the other side,
> > but completely messes up the subreg offset.  The following patch
> > appears to fix that problem:
>
> Indeed.  This should be ok to check in by itself.

Ah, maybe not: gen_lowpart probably isn't safe here as it may attempt
to generate new insns.  I think the appropriate thing to use here is
gen_lowpart_SUBREG.  OK with that change if testing passes?

> > I'm not sure how to fix that, not least because I still don't
understand
> > why this subreg is now supposed to be invalid in the first place ...
>
> According to the docs, it always was invalid.

Could you elaborate which docs?  I've been studying the 'subreg' section
in rtl.texi for quite a while, and been a bit frustrated by its attempt
to define things 'by example' instead of general statements ...

Thanks,
Ulrich

Re: [cft] subreg validation patch

[Richard Henderson]

On Mon, Nov 15, 2004 at 10:49:06PM +0100, Ulrich Weigand wrote:
> Ah, maybe not: gen_lowpart probably isn't safe here as it may attempt
> to generate new insns.  I think the appropriate thing to use here is
> gen_lowpart_SUBREG.  OK with that change if testing passes?

Hmm.  How about lowpart_subreg?  At least then we won't wind up
with nested subregs.

> Could you elaborate which docs?  I've been studying the 'subreg' section
> in rtl.texi for quite a while, and been a bit frustrated by its attempt
> to define things 'by example' instead of general statements ...

Nope.  On re-reading I don't see any such proscription.  I still
don't think it's a fantastic idea, but...


r~

Re: [cft] subreg validation patch

[Ulrich Weigand]

Richard Henderson <rth@redhat.com> wrote on 11/15/2004 11:02:32 PM:

> Hmm.  How about lowpart_subreg?  At least then we won't wind up
> with nested subregs.

src is guaranteed to satisfy REG_P at this point, so this shouldn't
be an issue.  (Also, regmove.c already uses gen_lowpart_SUBREG at
another similar location.)

> Nope.  On re-reading I don't see any such proscription.  I still
> don't think it's a fantastic idea, but...

I'm not necessarily arguing that (subreg:DF (reg:TI) 0) is something
we need to support for its own sake.  I'm only arguing that we need
clear rules, and preferably ones that are as simple as possible.
Given that I see no clear benefit to prohibiting that specific example,
simplicity of rules would appear to argue for allowing it ...

(In fact, for pseudos I would favor having no further restrictions
other than proper alignment at lowpart-of-subword boundary.  Hard
regs may have further restrictions, but reload should do the right
thing here.)


Bye,
Ulrich

Re: [cft] subreg validation patch

[Ulrich Weigand]

I wrote:

>Richard Henderson <rth@redhat.com> wrote on 11/15/2004 11:02:32 PM:
>
>> Hmm.  How about lowpart_subreg?  At least then we won't wind up
>> with nested subregs.
>
>src is guaranteed to satisfy REG_P at this point, so this shouldn't
>be an issue.  (Also, regmove.c already uses gen_lowpart_SUBREG at
>another similar location.)

On second thought, you're probably right: gen_lowpart_SUBREG may
abort (with your change), and we certainly don't want that here.
So I've changed the patch to use lowpart_subreg, and just ignore
that move if the subreg cannot be generated.

Bootstraped/regtested on s390-ibm-linux and s390x-ibm-linux
(with your patch applied as well).

OK for mainline?

Bye,
Ulrich


ChangeLog:

	* regmove.c (regmove_optimize): Use lowpart_subreg instead of
	gen_rtx_SUBREG with incorrect offset to compute SRC_SUBREG.

Index: gcc/regmove.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/regmove.c,v
retrieving revision 1.164
diff -c -p -r1.164 regmove.c
*** gcc/regmove.c	9 Oct 2004 19:19:24 -0000	1.164
--- gcc/regmove.c	16 Nov 2004 11:44:46 -0000
*************** regmove_optimize (rtx f, int nregs, FILE
*** 1150,1159 ****
  		  && GET_MODE_SIZE (GET_MODE (dst))
  		     >= GET_MODE_SIZE (GET_MODE (SUBREG_REG (dst))))
  		{
- 		  src_subreg
- 		    = gen_rtx_SUBREG (GET_MODE (SUBREG_REG (dst)),
- 				      src, SUBREG_BYTE (dst));
  		  dst = SUBREG_REG (dst);
  		}
  	      if (!REG_P (dst)
  		  || REGNO (dst) < FIRST_PSEUDO_REGISTER)
--- 1150,1160 ----
  		  && GET_MODE_SIZE (GET_MODE (dst))
  		     >= GET_MODE_SIZE (GET_MODE (SUBREG_REG (dst))))
  		{
  		  dst = SUBREG_REG (dst);
+ 		  src_subreg = lowpart_subreg (GET_MODE (dst),
+ 					       src, GET_MODE (src));
+ 		  if (!src_subreg)
+ 		    continue;
  		}
  	      if (!REG_P (dst)
  		  || REGNO (dst) < FIRST_PSEUDO_REGISTER)

-- 
  Dr. Ulrich Weigand
  Linux on zSeries Development
  Ulrich.Weigand@de.ibm.com

Re: [cft] subreg validation patch

[Richard Henderson]

On Tue, Nov 16, 2004 at 02:45:47PM +0100, Ulrich Weigand wrote:
> 	* regmove.c (regmove_optimize): Use lowpart_subreg instead of
> 	gen_rtx_SUBREG with incorrect offset to compute SRC_SUBREG.

Re: [cft] subreg validation patch

[Richard Henderson]

On Tue, Nov 16, 2004 at 02:45:47PM +0100, Ulrich Weigand wrote:
> 	* regmove.c (regmove_optimize): Use lowpart_subreg instead of
> 	gen_rtx_SUBREG with incorrect offset to compute SRC_SUBREG.

Err, ok.


r~