Re: PATCH RFC: My version of the lower-subreg patch

[Roger Sayle <roger at eyesopen dot com>]

Hi Ian,

On 15 Dec 2006, Ian Lance Taylor wrote:
> Various people have expressed interest in the lower-subreg patch over
> the past few months, so I'm sending out a copy.

Cool!  I've been your patch to fragments that I was investigating, and
wanted to ask you opinion on this bit, your changes to simplify-rtx.c:


> Index: gcc/simplify-rtx.c
> ===================================================================
> --- gcc/simplify-rtx.c	(revision 119834)
> +++ gcc/simplify-rtx.c	(working copy)
> @@ -4644,15 +4644,22 @@ simplify_subreg (enum machine_mode outer
>       && GET_MODE_SIZE (outermode) <= GET_MODE_SIZE (GET_MODE (op)))
>     return adjust_address_nv (op, outermode, byte);
>
> -  /* Handle complex values represented as CONCAT
> -     of real and imaginary part.  */
> -  if (GET_CODE (op) == CONCAT)
> +  /* Handle values represented as CONCAT.  */
> +  if (GET_CODE (op) == CONCAT || GET_CODE (op) == CONCATN)
>      {
>        unsigned int inner_size, final_offset;
>        rtx part, res;
>
> -      inner_size = GET_MODE_UNIT_SIZE (innermode);
> -      part = byte < inner_size ? XEXP (op, 0) : XEXP (op, 1);
> +      if (GET_CODE (op) == CONCAT)
> +	{
> +	  inner_size = GET_MODE_UNIT_SIZE (innermode);
> +	  part = byte < inner_size ? XEXP (op, 0) : XEXP (op, 1);
> +	}
> +      else
> +	{
> +	  inner_size = GET_MODE_SIZE (innermode) / XVECLEN (op, 0);
> +	  part = XVECEXP (op, 0, byte / inner_size);
> +	}
>       final_offset = byte % inner_size;
>       if (final_offset + GET_MODE_SIZE (outermode) > inner_size)
> 	return NULL_RTX;


I think the pre-existing logic here is incorrect.  Notice that for
CONCAT, we select which part we're talking about based upon whether
the byte offset is greater than innersize, which is the mode of the
CONCAT, and not of it's components.  In your new CONCATN case, you
correctly divide by XVECLEN.

One of the hunks to survive from my SUBREG lowering experiments is:

Index: simplify-rtx.c
===================================================================
--- simplify-rtx.c	(revision 113764)
+++ simplify-rtx.c	(working copy)
@@ -4580,13 +4580,22 @@
      of real and imaginary part.  */
   if (GET_CODE (op) == CONCAT)
     {
-      unsigned int inner_size, final_offset;
+      unsigned int part_size, final_offset;
       rtx part, res;

-      inner_size = GET_MODE_UNIT_SIZE (innermode);
-      part = byte < inner_size ? XEXP (op, 0) : XEXP (op, 1);
-      final_offset = byte % inner_size;
-      if (final_offset + GET_MODE_SIZE (outermode) > inner_size)
+      part_size = GET_MODE_UNIT_SIZE (XEXP (op, 0));
+      if (byte < part_size)
+        {
+          part = XEXP (op, 0);
+          final_offset = byte;
+	}
+      else
+	{
+	  part = XEXP (op, 1);
+	  final_offset = byte - part_size;
+	}
+
+      if (final_offset + GET_MODE_SIZE (outermode) > part_size)
 	return NULL_RTX;

       res = simplify_subreg (outermode, part, GET_MODE (part), final_offset);


where I use the mode of XEXP (op, 0) instead of innermode.


I think the reason why I was bitten by this, and you and RTH weren't,
is that in my efforts I didn't introduce a new CONCATN rtx, but instead
re-used all of the existing infrastructure to recursively lower SUBREGs
as nested CONCATs.  A TImode is a CONCAT of DImodes, where DI modes may
themselves be decomposed to CONCATs of SI modes, and so on.  Indeed on
AVR everything could end up being a tree of HImode or QImode.  Of course,
this only works for modes that are powers of two, but for GCC this
allowed me to probe the issues.  Indeed the generalization above now
supports CONCATs where the first and second components potentially have
different modes/sizes!


Do you agree this is a latent bug?  Any opinions on CONCATN vs.
nested CONCAT?  [Hint: I didn't need to touch reload! :-)]

Roger
--