cprop_reg problem on sparc

cprop_reg problem on sparc

[David Miller]

Although copy_value() in regcprop.c tries to avoid recording cases
where substitutions would be illegal, there are some bad cases it
still can let through.

On 64-bit sparc, integer regs are 64-bit and float regs are
(basically) 32-bit.  So HARD_REGNO_NREGS(float_reg, DFmode) is 2, and
HARD_REGNO_NREGS(integer_reg, DImode) is 1.

cprop sees the sequence:

(insn 330 172 230 .. (set (reg:DI %g2) const_int)
(insn 171 330 173 .. (set (reg:DF %f10) (reg:DF %g2)))
(insn 173 171 222 .. (set (reg:DF %f2) (reg:DF %f10)))
(insn 222 173 223 .. (set (MEM:SI ..) (reg:SI %f10)))
(insn 223 222 174 .. (set (MEM:SI ..) (reg:SI %f11)))

And then it believes that in insn 222 it can replace %f10 with %g2,
but this is not a correct transformation.

cprop uses hard_regno_nregs[][] to attempt to detect illegal cases
like this one, but such checks will not trigger here because
hard_regno_nregs[][] is '1' for all of the registers being inspected:

	hard_regno_nregs[][] (reg:SI f10)	1
	hard_regno_nregs[][] (reg:DI g2)	1

The (set (reg:DI %g2) const_int) is generated by the *movdf_insn_sp64 insn
which in turn triggers a splitter for loading float constants into
integer registers.

The MEM:SI stores are reloads generated by IRA for a pseudo that has
to live across a call.  For whatever reason it allocated only a 4-byte
aligned stack location, and I suppose that is why the reload is split
into 2 SImode pieces.

To reproduce build gcc.c-torture/execute/ieee/mzero.c with
"-m64 -mcpu=niagara3 -O2" on sparc.

I'm suspecting that perhaps cprop is ok, and the real issue is that
sparc's definition of CANNOT_CHANGE_MODE_CLASS needs to be adjusted.

Re: cprop_reg problem on sparc

[Eric Botcazou]

> On 64-bit sparc, integer regs are 64-bit and float regs are
> (basically) 32-bit.  So HARD_REGNO_NREGS(float_reg, DFmode) is 2, and
> HARD_REGNO_NREGS(integer_reg, DImode) is 1.
>
> cprop sees the sequence:
>
> (insn 330 172 230 .. (set (reg:DI %g2) const_int)
> (insn 171 330 173 .. (set (reg:DF %f10) (reg:DF %g2)))
> (insn 173 171 222 .. (set (reg:DF %f2) (reg:DF %f10)))
> (insn 222 173 223 .. (set (MEM:SI ..) (reg:SI %f10)))
> (insn 223 222 174 .. (set (MEM:SI ..) (reg:SI %f11)))
>
> And then it believes that in insn 222 it can replace %f10 with %g2,
> but this is not a correct transformation.
>
> cprop uses hard_regno_nregs[][] to attempt to detect illegal cases
> like this one, but such checks will not trigger here because
> hard_regno_nregs[][] is '1' for all of the registers being inspected:
>
> 	hard_regno_nregs[][] (reg:SI f10)	1
> 	hard_regno_nregs[][] (reg:DI g2)	1

There seems to be a hole in the checks, as the number of registers is 2 for 
some of the intermediate steps.

> To reproduce build gcc.c-torture/execute/ieee/mzero.c with
> "-m64 -mcpu=niagara3 -O2" on sparc.

AFAICS there is no such file as gcc.c-torture/execute/ieee/mzero.c.

> I'm suspecting that perhaps cprop is ok, and the real issue is that
> sparc's definition of CANNOT_CHANGE_MODE_CLASS needs to be adjusted.

I'm a little skeptical at this point.

-- 
Eric Botcazou

Re: cprop_reg problem on sparc

[David Miller]

From: Eric Botcazou <ebotcazou@adacore.com>
Date: Thu, 27 Oct 2011 15:17:40 +0200

>> To reproduce build gcc.c-torture/execute/ieee/mzero.c with
>> "-m64 -mcpu=niagara3 -O2" on sparc.
> 
> AFAICS there is no such file as gcc.c-torture/execute/ieee/mzero.c.

Sorry, the final path component should be "mzero2.c"

Re: cprop_reg problem on sparc

[Eric Botcazou]

> Sorry, the final path component should be "mzero2.c"

Thanks.  I think we that need the same treatment in:

	  /* If we are accessing SRC in some mode other that what we
	     set it in, make sure that the replacement is valid.  */
	  if (mode != vd->e[regno].mode)
	    {
	      if (hard_regno_nregs[regno][mode]
		  > hard_regno_nregs[regno][vd->e[regno].mode])
		goto no_move_special_case;
	    }

as:

  /* If we are narrowing the input to a smaller number of hard regs,
     and it is in big endian, we are really extracting a high part.
     Since we generally associate a low part of a value with the value itself,
     we must not do the same for the high part.
     Note we can still get low parts for the same mode combination through
     a two-step copy involving differently sized hard regs.
     Assume hard regs fr* are 32 bits bits each, while r* are 64 bits each:
     (set (reg:DI r0) (reg:DI fr0))
     (set (reg:SI fr2) (reg:SI r0))
     loads the low part of (reg:DI fr0) - i.e. fr1 - into fr2, while:
     (set (reg:SI fr2) (reg:SI fr0))
     loads the high part of (reg:DI fr0) into fr2.

     We can't properly represent the latter case in our tables, so don't
     record anything then.  */
  else if (sn < (unsigned int) hard_regno_nregs[sr][vd->e[sr].mode]
	   && (GET_MODE_SIZE (vd->e[sr].mode) > UNITS_PER_WORD
	       ? WORDS_BIG_ENDIAN : BYTES_BIG_ENDIAN))
    return;

i.e. we need to bail out if we are narrowing and this is a big-endian target.

-- 
Eric Botcazou

Re: cprop_reg problem on sparc

[David Miller]

From: Eric Botcazou <ebotcazou@adacore.com>
Date: Thu, 27 Oct 2011 23:11:33 +0200

>> Sorry, the final path component should be "mzero2.c"
> 
> Thanks.  I think we that need the same treatment in:
 ...
> as:
 ...
> i.e. we need to bail out if we are narrowing and this is a big-endian target.

I quickly tried the patch below, but this does not prevent the
transformation.

diff --git a/gcc/regcprop.c b/gcc/regcprop.c
index ad92a64..54e008b 100644
--- a/gcc/regcprop.c
+++ b/gcc/regcprop.c
@@ -448,6 +448,14 @@ find_oldest_value_reg (enum reg_class cl, rtx reg, struct value_data *vd)
       if (hard_regno_nregs[regno][mode]
 	  > hard_regno_nregs[regno][vd->e[regno].mode])
 	return NULL_RTX;
+
+      /* And likewise, if we are narrowing on big endian the transformation
+	 is also invalid.  */
+      if (hard_regno_nregs[regno][mode]
+	  < hard_regno_nregs[regno][vd->e[regno].mode]
+	  && (GET_MODE_SIZE (vd->e[regno].mode) > UNITS_PER_WORD
+	      ? WORDS_BIG_ENDIAN : BYTES_BIG_ENDIAN))
+	return NULL_RTX;
     }
 
   for (i = vd->e[regno].oldest_regno; i != regno; i = vd->e[i].next_regno)

Re: cprop_reg problem on sparc

[Eric Botcazou]

> I quickly tried the patch below, but this does not prevent the
> transformation.

The quoted code is in copyprop_hardreg_forward_1.

-- 
Eric Botcazou

Re: cprop_reg problem on sparc

[David Miller]

From: Eric Botcazou <ebotcazou@adacore.com>
Date: Thu, 27 Oct 2011 23:55:00 +0200

>> I quickly tried the patch below, but this does not prevent the
>> transformation.
> 
> The quoted code is in copyprop_hardreg_forward_1.

Indeed :-)

This patch below works for the specific test case, and I'll post to
gcc-patches and commit it after regstrapping.

Thanks Eric!

--------------------
Fix illegal register substitutions on big-endian during cprop_reg.

	* regcprop.c (copyprop_hardreg_forward_1): Reject the
	transformation when we narrow the mode on big endian.
---
 gcc/ChangeLog  |    5 +++++
 gcc/regcprop.c |    8 ++++++++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 403fb60..54e059e 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@
+2011-10-27  David S. Miller  <davem@davemloft.net>
+
+	* regcprop.c (copyprop_hardreg_forward_1): Reject the
+	transformation when we narrow the mode on big endian.
+
 2011-10-27  Jakub Jelinek  <jakub@redhat.com>
 
 	* config/i386/sse.md (avx_cvtpd2dq256_2, avx_cvttpd2dq256_2,
diff --git a/gcc/regcprop.c b/gcc/regcprop.c
index ad92a64..b0f0343 100644
--- a/gcc/regcprop.c
+++ b/gcc/regcprop.c
@@ -824,6 +824,14 @@ copyprop_hardreg_forward_1 (basic_block bb, struct value_data *vd)
 	      if (hard_regno_nregs[regno][mode]
 		  > hard_regno_nregs[regno][vd->e[regno].mode])
 		goto no_move_special_case;
+
+	      /* And likewise, if we are narrowing on big endian the transformation
+		 is also invalid.  */
+	      if (hard_regno_nregs[regno][mode]
+		  < hard_regno_nregs[regno][vd->e[regno].mode]
+		  && (GET_MODE_SIZE (vd->e[regno].mode) > UNITS_PER_WORD
+		      ? WORDS_BIG_ENDIAN : BYTES_BIG_ENDIAN))
+		goto no_move_special_case;
 	    }
 
 	  /* If the destination is also a register, try to find a source
-- 
1.7.6.401.g6a319

Re: cprop_reg problem on sparc

[David Miller]

From: Eric Botcazou <ebotcazou@adacore.com>
Date: Thu, 27 Oct 2011 15:17:40 +0200

>> On 64-bit sparc, integer regs are 64-bit and float regs are
>> (basically) 32-bit.  So HARD_REGNO_NREGS(float_reg, DFmode) is 2, and
>> HARD_REGNO_NREGS(integer_reg, DImode) is 1.
>>
>> cprop sees the sequence:
>>
>> (insn 330 172 230 .. (set (reg:DI %g2) const_int)
>> (insn 171 330 173 .. (set (reg:DF %f10) (reg:DF %g2)))
>> (insn 173 171 222 .. (set (reg:DF %f2) (reg:DF %f10)))
>> (insn 222 173 223 .. (set (MEM:SI ..) (reg:SI %f10)))
>> (insn 223 222 174 .. (set (MEM:SI ..) (reg:SI %f11)))
>>
>> And then it believes that in insn 222 it can replace %f10 with %g2,
>> but this is not a correct transformation.
>>
>> cprop uses hard_regno_nregs[][] to attempt to detect illegal cases
>> like this one, but such checks will not trigger here because
>> hard_regno_nregs[][] is '1' for all of the registers being inspected:
>>
>> 	hard_regno_nregs[][] (reg:SI f10)	1
>> 	hard_regno_nregs[][] (reg:DI g2)	1
> 
> There seems to be a hole in the checks, as the number of registers is 2 for 
> some of the intermediate steps.

I think cprop_reg can legitimately only consider the candidate read
replacement (reg:SI %f10) with the most recent store to it's
equivalent value (reg:DI %g2).

In fact, that's exactly what cprop_reg's core algorithm is :-)

The issue is how to test properly that accesses to a value in two
register accesses is equivalent after a move.  This pass is currently
using hard_regno_nregs[][] and a paradoxical subreg test to achieve
that, but it's obviously not sufficient.

Note that it would actually be legal to make the transformation on
insn 223, replacing %f11 with %g2.

But I'll note that if IRA had properly spilled %f10 to the stack using
an 8-byte aligned stack slot and DFmode, we wouldn't even be having to
consider this situation.

I think cprop_reg should cope with this properly, but I also think that
it would be nice if we worked to minimize unnecessary mode changes like
those seen here.  The const-->DFmode splitter in sparc.md is partly to
blame, as is IRA.