RFA: Fix bogus mode in choose_reload_regs

RFA: Fix bogus mode in choose_reload_regs

[Richard Sandiford]

This patch fixes an ICE in smallest_mode_for_size on the attached testcase.
The smallest_mode_for_size call comes from this part of the reload
inheritance code in choose_reload_regs:

		  if (byte == 0)
		    need_mode = mode;
		  else
		    need_mode
		      = smallest_mode_for_size
		        (GET_MODE_BITSIZE (mode) + byte * BITS_PER_UNIT,
			 GET_MODE_CLASS (mode) == MODE_PARTIAL_INT
			 ? MODE_INT : GET_MODE_CLASS (mode));

		  if ((GET_MODE_SIZE (GET_MODE (last_reg))
		       >= GET_MODE_SIZE (need_mode))

Here we have found that the pseudo register we need was last reloaded
into LAST_REG.  The mode size check is making sure LAST_REG defines
every byte of the value we need (which is at byte offset BYTE and
has mode MODE).

In the attached testcase, LAST_REG is XImode (a 256-bit integer),
and the value we need is the last vector quarter of it.  BYTE is 24
and MODE is V4SF.  The problem is that we then look for a 256-bit vector:

    smallest_mode_for_size (64 + 24 * 8, MODE_VECTOR_FLOAT)

but no such mode exists.

Note that this is the only use of need_mode.  I don't believe the mode
that is being calculated here is fundamental in any way, or that it's
used later in the reload process.  We have already checked that the mode
change is allowed:

#ifdef CANNOT_CHANGE_MODE_CLASS
		  /* Verify that the register it's in can be used in
		     mode MODE.  */
		  && !REG_CANNOT_CHANGE_MODE_P (REGNO (reg_last_reload_reg[regno]),
						GET_MODE (reg_last_reload_reg[regno]),
						mode)
#endif

and have already calculated which hard register we would need to
use after the mode change:

		  i = REGNO (last_reg);
		  i += subreg_regno_offset (i, GET_MODE (last_reg), byte, mode);

So once we have verified that the register is suitable, we can (and do)
simply use register I in mode MODE.

I think the current mode is a historical left-over.  Back in 2000 this code
was a simple check that the old register entirely encompassed the new one:

		  i = REGNO (last_reg) + word;
		  last_class = REGNO_REG_CLASS (i);
		  if ((GET_MODE_SIZE (GET_MODE (last_reg))
		       >= GET_MODE_SIZE (mode) + word * UNITS_PER_WORD)

The register we were interested in was (reg:MODE I), and this check made
sure that the old reload register defined every byte of (reg:MODE I).
When CLASS_CANNOT_CHANGE_SIZE was introduced, the code became:

		  i = REGNO (last_reg) + word;
		  last_class = REGNO_REG_CLASS (i);
		  if (
#ifdef CLASS_CANNOT_CHANGE_SIZE
		      (TEST_HARD_REG_BIT
		       (reg_class_contents[CLASS_CANNOT_CHANGE_SIZE], i)
		       ? (GET_MODE_SIZE (GET_MODE (last_reg))
			  == GET_MODE_SIZE (mode) + word * UNITS_PER_WORD)
		       : (GET_MODE_SIZE (GET_MODE (last_reg))
			  >= GET_MODE_SIZE (mode) + word * UNITS_PER_WORD))
#else
		      (GET_MODE_SIZE (GET_MODE (last_reg))
		       >= GET_MODE_SIZE (mode) + word * UNITS_PER_WORD)
#endif

But I think this was bogus.  The new size of the register was:

   GET_MODE_SIZE (mode)

rather than:

   GET_MODE_SIZE (mode) + word * UNITS_PER_WORD

Maybe something like:

   word == 0 && GET_MODE_SIZE (mode) == GET_MODE_SIZE (GET_MODE (last_reg))

would have been more accurate.  Anyway, CLASS_CANNOT_CHANGE_SIZE proved
to be too limited, so it was replaced with CLASS_CANNOT_CHANGE_MODE.
The code above then became:

		  need_mode = smallest_mode_for_size ((word+1) * UNITS_PER_WORD,
						      GET_MODE_CLASS (mode));

		  if (
#ifdef CLASS_CANNOT_CHANGE_MODE
		      (TEST_HARD_REG_BIT
		       (reg_class_contents[(int) CLASS_CANNOT_CHANGE_MODE], i)
		       ? ! CLASS_CANNOT_CHANGE_MODE_P (GET_MODE (last_reg), 
						       need_mode)
		       : (GET_MODE_SIZE (GET_MODE (last_reg))
			  >= GET_MODE_SIZE (need_mode)))
#else
		      (GET_MODE_SIZE (GET_MODE (last_reg))
		       >= GET_MODE_SIZE (need_mode))
#endif

with need_mode providing a mode of the same size as the then-preexisting
size check.  I think this mode is bogus for the same reason, and in 2005
I changed the final mode argument from "need_mode" to "mode":

    http://gcc.gnu.org/ml/gcc-patches/2005-02/msg01665.html

That patch also fixed the smallest_mode_for_size argument so that it
was a bit count rather than a byte count.  Unfortunately, it seems
I failed to realise that need_mode was in fact completely meaningless,
and should have just been removed instead.  Indeed, the bit->byte fix
exposed a bug very like this one on s390:

    http://gcc.gnu.org/ml/gcc-patches/2005-04/msg01226.html

Ulrich wisely said:

    As real fix, I think the computation of a "needed mode" may be
    completely superfluous in the first place; instead, the first
    clause of the 'if' could just be replaced by

      if ((GET_MODE_SIZE (GET_MODE (last_reg))
	   >= GET_MODE_SIZE (mode) + byte)

to which I whined:

    I remember wondering about this too.  Unfortunately, there are no
    comments at all explaining what the check is actually supposed to
    do, or what "need_mode" is suppsoed to be, so I thought at the time
    it was best to leave things be.

But we were in release-paranoia mode, and that real fix never happened.
I agree that removing need_mode is the right fix for that s390 PR,
and for this NEON one.

For completeness, Vlad hit a similar problem during the IRA work,
which is whether the MODE_PARTIAL_INT check came from:

  http://gcc.gnu.org/ml/gcc-patches/2008-12/msg00265.html

Vlad too thought that this code was probably bogus.

Tested on x86_64-linux-gnu and arm-linux-gnueabi.  OK to install?

Richard


gcc/
	* reload1.c (choose_reload_regs): Use mode sizes to check whether
	an old relaod register completely defines the required value.

gcc/testsuite/
	* gcc.target/arm/neon-modes-3.c: New test.

Index: gcc/reload1.c
===================================================================
--- gcc/reload1.c	2011-07-05 13:15:27.000000000 +0100
+++ gcc/reload1.c	2011-07-05 13:49:02.000000000 +0100
@@ -6485,6 +6485,8 @@ choose_reload_regs (struct insn_chain *c
 
 	      if (regno >= 0
 		  && reg_last_reload_reg[regno] != 0
+		  && (GET_MODE_SIZE (GET_MODE (reg_last_reload_reg[regno]))
+		      >= GET_MODE_SIZE (mode) + byte)
 #ifdef CANNOT_CHANGE_MODE_CLASS
 		  /* Verify that the register it's in can be used in
 		     mode MODE.  */
@@ -6496,24 +6498,12 @@ choose_reload_regs (struct insn_chain *c
 		{
 		  enum reg_class rclass = rld[r].rclass, last_class;
 		  rtx last_reg = reg_last_reload_reg[regno];
-		  enum machine_mode need_mode;
 
 		  i = REGNO (last_reg);
 		  i += subreg_regno_offset (i, GET_MODE (last_reg), byte, mode);
 		  last_class = REGNO_REG_CLASS (i);
 
-		  if (byte == 0)
-		    need_mode = mode;
-		  else
-		    need_mode
-		      = smallest_mode_for_size
-		        (GET_MODE_BITSIZE (mode) + byte * BITS_PER_UNIT,
-			 GET_MODE_CLASS (mode) == MODE_PARTIAL_INT
-			 ? MODE_INT : GET_MODE_CLASS (mode));
-
-		  if ((GET_MODE_SIZE (GET_MODE (last_reg))
-		       >= GET_MODE_SIZE (need_mode))
-		      && reg_reloaded_contents[i] == regno
+		  if (reg_reloaded_contents[i] == regno
 		      && TEST_HARD_REG_BIT (reg_reloaded_valid, i)
 		      && HARD_REGNO_MODE_OK (i, rld[r].mode)
 		      && (TEST_HARD_REG_BIT (reg_class_contents[(int) rclass], i)
Index: gcc/testsuite/gcc.target/arm/neon-modes-3.c
===================================================================
--- /dev/null	2011-07-05 08:57:30.268785575 +0100
+++ gcc/testsuite/gcc.target/arm/neon-modes-3.c	2011-07-05 13:48:54.000000000 +0100
@@ -0,0 +1,61 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target arm_neon_ok } */
+/* { dg-options "-O" } */
+/* { dg-add-options arm_neon } */
+
+#include <arm_neon.h>
+
+void f1 (volatile float32x4_t *dest, volatile float32x4x4_t *src, int n)
+{
+  float32x4x4_t a5, a6, a7, a8, a9;
+  int i;
+
+  a5 = *src;
+  a6 = *src;
+  a7 = *src;
+  a8 = *src;
+  a9 = *src;
+  while (n--)
+    {
+      for (i = 0; i < 8; i++)
+	{
+	  float32x4x4_t a0, a1, a2, a3, a4;
+
+	  a0 = *src;
+	  a1 = *src;
+	  a2 = *src;
+	  a3 = *src;
+	  a4 = *src;
+	  *src = a0;
+	  *dest = a0.val[0];
+	  *dest = a0.val[3];
+	  *src = a1;
+	  *dest = a1.val[0];
+	  *dest = a1.val[3];
+	  *src = a2;
+	  *dest = a2.val[0];
+	  *dest = a2.val[3];
+	  *src = a3;
+	  *dest = a3.val[0];
+	  *dest = a3.val[3];
+	  *src = a4;
+	  *dest = a4.val[0];
+	  *dest = a4.val[3];
+	}
+      *src = a5;
+      *dest = a5.val[0];
+      *dest = a5.val[3];
+      *src = a6;
+      *dest = a6.val[0];
+      *dest = a6.val[3];
+      *src = a7;
+      *dest = a7.val[0];
+      *dest = a7.val[3];
+      *src = a8;
+      *dest = a8.val[0];
+      *dest = a8.val[3];
+      *src = a9;
+      *dest = a9.val[0];
+      *dest = a9.val[3];
+    }
+}

Re: RFA: Fix bogus mode in choose_reload_regs

[Ulrich Weigand]

Richard Sandiford wrote:

> gcc/
> 	* reload1.c (choose_reload_regs): Use mode sizes to check whether
> 	an old relaod register completely defines the required value.
> 
> gcc/testsuite/
> 	* gcc.target/arm/neon-modes-3.c: New test.

This is OK.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: RFA: Fix bogus mode in choose_reload_regs

[Jay Foad]

On 7 July 2011 09:09, Richard Sandiford <richard.sandiford@linaro.org> wrote:
> gcc/
>        * reload1.c (choose_reload_regs): Use mode sizes to check whether
>        an old relaod register completely defines the required value.

s/relaod/reload/

Jay.