This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] More thorough checking in reg_fits_class_p

On 30/04/12 16:19, Richard Sandiford wrote:
Richard Earnshaw<rearnsha@arm.com> writes:
On 30/04/12 15:39, Richard Sandiford wrote:
Richard Earnshaw<rearnsha@arm.com> writes:
On 30/04/12 15:07, Richard Sandiford wrote:
Richard Earnshaw<rearnsha@arm.com> writes:
On 26/04/12 14:20, Jim MacArthur wrote: 
The current code in reg_fits_class_p appears to be incorrect; since
offset may be negative, it's necessary to check both ends of the range
otherwise an array overrun or underrun may occur when calling
in_hard_reg_set_p. in_hard_reg_set_p should also be checked for each
register in the range of regno .. regno+offset.

A negative offset can occur on a big-endian target. For example, on
AArch64, subreg_regno_offset (0, DImode, 0, TImode) returns -1.

We discovered this problem while developing unrelated code for
big-endian support in the AArch64 back end.

I've tested this with an x86 bootstrap which shows no errors, and with
our own AArch64 back end.

Ok for trunk?

gcc/Changelog entry:

2012-04-26 Jim MacArthur<jim.macarthur@arm.com>
       * recog.c (reg_fits_class_p): Check every register between regno and
         regno+offset is in the hard register set.

OK.

R.

reg-fits-class-9


diff --git a/gcc/recog.c b/gcc/recog.c
index 8fb96a0..825bfb1 100644
--- a/gcc/recog.c
+++ b/gcc/recog.c
@@ -2759,14 +2759,28 @@ bool
  reg_fits_class_p (const_rtx operand, reg_class_t cl, int offset,
  		  enum machine_mode mode)
  {
-  int regno = REGNO (operand);
+  unsigned int regno = REGNO (operand);

    if (cl == NO_REGS)
      return false;

-  return (HARD_REGISTER_NUM_P (regno)
-	&&  in_hard_reg_set_p (reg_class_contents[(int) cl],
-				mode, regno + offset));
+  /* We should not assume all registers in the range regno to regno + offset are
+     valid just because each end of the range is.  */
+  if (HARD_REGISTER_NUM_P (regno)&&  HARD_REGISTER_NUM_P (regno + offset))
+    {
+      unsigned int i;
+
+      unsigned int start = MIN (regno, regno + offset);
+      unsigned int end = MAX (regno, regno + offset);
+      for (i = start; i<= end; i++)
+	{
+	  if (!in_hard_reg_set_p (reg_class_contents[(int) cl],
+				  mode, i))
+	    return false;
+	}

This doesn't look right to me.  We should still only need to check
in_hard_reg_set_p for one register number.  I'd have expected
something like:

   return (HARD_REGISTER_NUM_P (regno)
	&&  HARD_REGISTER_NUM_P (regno + offset)
	&&  in_hard_reg_set_p (reg_class_contents[(int) cl],
				mode, regno + offset));

instead.

Richard

There's no guarantee that all registers in a set are contiguous; ARM for
example doesn't make that guarantee, since SP is not a GP register, but
R12 and R14 are.

Sorry, I don't follow.  My point was that in_hard_reg_set_p (C, M, R1)
tests whether every register required to store a value of mode M starting
at R1 fits in C.  Which is what we want to know.

Whether the intermediate registers (between regno and regno + offset)
are even valid for MODE shouldn't matter.  I don't think it makes
conceptual sense to call:

	  if (!in_hard_reg_set_p (reg_class_contents[(int) cl],
				  mode, i))

for regno<  i<  regno + offset (or regno + offset<  i<  regno),
because we're not trying to construct a value of mode MODE
in that register.

Richard


You're right, of course.  I'd missed that in my initial review; and
hence my follow-up suggestion.  It's not particularly interesting
whether or not HARD_REGISTER_NUM_P (REGNO (operand)) is true, only
whether REGNO(operand) + offset ... REGNO(operand) + offset +
NUM_REGS(mode) -1 is.

The problem is that the function currently accepts pseudo registers.
We wouldn't want FIRST_PSEUDO_REGISTER to be accidentally associated
with FIRST_PSUEDO_REGISTER-1, however unlikely that combination of
arguments might be in practice.  I think the HARD_REGISTER_NUM_P check
is needed for both regno and regno + offset.

I agree that we should protect against overrun in in_hard_reg_set,
but I think that check logically belongs there.  Most targets happen
to be OK because FIRST_PSEUDO_REGISTER isn't divisible by 32,
so the class HARD_REG_SETs always have a terminating zero bit.
There are a couple of exceptions though, such as alpha and lm32.

So one fix would be to require HARD_REG_SET to have an entry
for FIRST_PSEUDO_REGISTER, which wouldn't affect most targets.
Another would be to have an explicit range check in in_hard_reg_set_p
and friends.

Richard


Thanks for all your comments. I've made a new version which checks all three cases (regno, regno+offset, end_hard_regno (regno+offset, M) ) with HARD_REGISTER_NUM_P but returns to just checking regno + offset with in_hard_reg_set_p. How does this look?

New Changelog text:

2012-05-02 Jim MacArthur<jim.macarthur@arm.com>
* recog.c (reg_fits_class_p): Check both regno and regno + offset are
hard registers.


Attachment: reg-fits-class-10
Description: Text document

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]