[Patch, MIPS] Patch for PR 68400, a mips16 bug

[Patch, MIPS] Patch for PR 68400, a mips16 bug

[Steve Ellcey]

Here is a patch for PR6400.  The problem is that and_operands_ok was checking
one operand to see if it was a memory_operand but MIPS16 addressing is more
restrictive than what the general memory_operand allows.  The fix was to
call mips_classify_address if TARGET_MIPS16 is set because it will do a
more complete mips16 addressing check and reject operands that do not meet
the more restrictive requirements.

I ran the GCC testsuite with no regressions and have included a test case as
part of this patch.

OK to checkin?

Steve Ellcey
sellcey@imgtec.com


2016-01-26  Steve Ellcey  <sellcey@imgtec.com>

	PR target/68400
	* config/mips/mips.c (and_operands_ok): Add MIPS16 check.



diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index dd54d6a..adeafa3 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -8006,9 +8006,18 @@ mask_low_and_shift_p (machine_mode mode, rtx mask, rtx shift, int maxlen)
 bool
 and_operands_ok (machine_mode mode, rtx op1, rtx op2)
 {
-  return (memory_operand (op1, mode)
-	  ? and_load_operand (op2, mode)
-	  : and_reg_operand (op2, mode));
+
+  if (memory_operand (op1, mode))
+    {
+      if (TARGET_MIPS16) {
+	struct mips_address_info addr;
+	if (!mips_classify_address (&addr, op1, mode, false))
+	  return false;
+      }
+      return and_load_operand (op2, mode);
+    }
+  else
+    return and_reg_operand (op2, mode);
 }
 
 /* The canonical form of a mask-low-and-shift-left operation is




2016-01-26  Steve Ellcey  <sellcey@imgtec.com>

	PR target/68400
	* gcc.target/mips/mips.exp (mips_option_groups): Add stack-protector.
	* gcc.target/mips/pr68400.c: New test.


diff --git a/gcc/testsuite/gcc.target/mips/mips.exp b/gcc/testsuite/gcc.target/mips/mips.exp
index f191331..ff9c99a 100644
--- a/gcc/testsuite/gcc.target/mips/mips.exp
+++ b/gcc/testsuite/gcc.target/mips/mips.exp
@@ -257,6 +257,7 @@ set mips_option_groups {
     lsa "(|!)HAS_LSA"
     section_start "-Wl,--section-start=.*"
     frame-header "-mframe-header-opt|-mno-frame-header-opt"
+    stack-protector "-fstack-protector"
 }
 
 for { set option 0 } { $option < 32 } { incr option } {
diff --git a/gcc/testsuite/gcc.target/mips/pr68400.c b/gcc/testsuite/gcc.target/mips/pr68400.c
index e69de29..1099568 100644
--- a/gcc/testsuite/gcc.target/mips/pr68400.c
+++ b/gcc/testsuite/gcc.target/mips/pr68400.c
@@ -0,0 +1,28 @@
+/* PR target/pr68400
+   This was triggering an ICE in change_address_1 when compiled with -Os.  */
+
+/* { dg-do compile } */
+/* { dg-options "-fstack-protector -mips16" } */
+
+typedef struct s {
+ unsigned long long d;
+ long long t;
+} p;
+
+int sh(int x, unsigned char *buf)
+{
+ p *uhdr = (p *)buf;
+ unsigned int i = 0;
+ uhdr->d = ((uhdr->d & 0xff00000000000000LL) >> 56)
+            | ((uhdr->d & 0x0000ff0000000000LL) >> 24)
+            | ((uhdr->d & 0x00000000ff000000LL) << 8)
+            | ((uhdr->d & 0x00000000000000ffLL) << 56);
+ uhdr->t = ((uhdr->t & 0xff00000000000000LL) >> 56)
+                | ((uhdr->t & 0x0000ff0000000000LL) >> 24)
+                | ((uhdr->t & 0x000000ff00000000LL) >> 8)
+                | ((uhdr->t & 0x00000000ff000000LL) << 8)
+                | ((uhdr->t & 0x000000000000ff00LL) << 40)
+                | ((uhdr->t & 0x00000000000000ffLL) << 56);
+ i += 4;
+ if (x < i) return 0; else return 1;
+}

RE: [Patch, MIPS] Patch for PR 68400, a mips16 bug

[Moore, Catherine]

> -----Original Message-----
> From: Steve Ellcey [mailto:sellcey@imgtec.com]
> Sent: Tuesday, January 26, 2016 3:43 PM
> To: gcc-patches@gcc.gnu.org
> Cc: matthew.fortune@imgtec.com; Moore, Catherine
> Subject: [Patch, MIPS] Patch for PR 68400, a mips16 bug
> 
> Here is a patch for PR6400.  The problem is that and_operands_ok was
> checking one operand to see if it was a memory_operand but MIPS16
> addressing is more restrictive than what the general memory_operand
> allows.  The fix was to call mips_classify_address if TARGET_MIPS16 is set
> because it will do a more complete mips16 addressing check and reject
> operands that do not meet the more restrictive requirements.
> 
> I ran the GCC testsuite with no regressions and have included a test case as
> part of this patch.
> 
> OK to checkin?
> 
> Steve Ellcey
> sellcey@imgtec.com
> 
> 
> 2016-01-26  Steve Ellcey  <sellcey@imgtec.com>
> 
> 	PR target/68400
> 	* config/mips/mips.c (and_operands_ok): Add MIPS16 check.
> 
> 
> 
This is OK.
Thanks, Catherine

Re: [Patch, MIPS] Patch for PR 68400, a mips16 bug

[Richard Sandiford]

"Steve Ellcey " <sellcey@imgtec.com> writes:
> Here is a patch for PR6400.  The problem is that and_operands_ok was checking
> one operand to see if it was a memory_operand but MIPS16 addressing is more
> restrictive than what the general memory_operand allows.  The fix was to
> call mips_classify_address if TARGET_MIPS16 is set because it will do a
> more complete mips16 addressing check and reject operands that do not meet
> the more restrictive requirements.
>
> I ran the GCC testsuite with no regressions and have included a test case as
> part of this patch.
>
> OK to checkin?
>
> Steve Ellcey
> sellcey@imgtec.com
>
>
> 2016-01-26  Steve Ellcey  <sellcey@imgtec.com>
>
> 	PR target/68400
> 	* config/mips/mips.c (and_operands_ok): Add MIPS16 check.
>
>
>
> diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
> index dd54d6a..adeafa3 100644
> --- a/gcc/config/mips/mips.c
> +++ b/gcc/config/mips/mips.c
> @@ -8006,9 +8006,18 @@ mask_low_and_shift_p (machine_mode mode, rtx mask, rtx shift, int maxlen)
>  bool
>  and_operands_ok (machine_mode mode, rtx op1, rtx op2)
>  {
> -  return (memory_operand (op1, mode)
> -	  ? and_load_operand (op2, mode)
> -	  : and_reg_operand (op2, mode));
> +
> +  if (memory_operand (op1, mode))
> +    {
> +      if (TARGET_MIPS16) {
> +	struct mips_address_info addr;
> +	if (!mips_classify_address (&addr, op1, mode, false))
> +	  return false;
> +      }

Nit: brace formatting.

It looks like the patch only works by accident.  The code above
is passing the MEM, rather than the address inside the MEM, to
mips_classify_address.  Since (mem (mem ...)) is never valid on MIPS,
the effect is to disable the memory alternatives of *and<mode>3_mips16
unconditionally.

The addresses that occur in the testcase are valid as far as
mips_classify_address is concerned.  FWIW, there shouldn't be any
difference between the addresses that memory_operand accepts and the
addresses that mips_classify_address accepts.

In theory, the "W" constraints in *and<mode>3_mips16 are supposed to
tell the target-independent code that this instruction cannot handle
constant addresses or stack-based addresses.  That seems to be working
correctly during RA for the testcase.  The problem comes in regcprop,
which ends up creating a second stack pointer rtx distinct from
stack_pointer_rtx:

    (reg/f:SI 29 $sp [375])

(Note the ORIGINAL_REGNO of 375.)  This then defeats the test in
mips_stack_address_p:

bool
mips_stack_address_p (rtx x, machine_mode mode)
{
  struct mips_address_info addr;

  return (mips_classify_address (&addr, x, mode, false)
	  && addr.type == ADDRESS_REG
	  && addr.reg == stack_pointer_rtx);
}

Change the == to rtx_equal_p and the test passes.  I don't think that's
the correct fix though -- the fix is to stop a second stack pointer rtx
from being created.

Thanks,
Richard

RE: [Patch, MIPS] Patch for PR 68400, a mips16 bug

[Matthew Fortune]

Richard Sandiford <rdsandiford@googlemail.com> writes:
> "Steve Ellcey " <sellcey@imgtec.com> writes:
> > Here is a patch for PR6400.  The problem is that and_operands_ok was checking
> > one operand to see if it was a memory_operand but MIPS16 addressing is more
> > restrictive than what the general memory_operand allows.  The fix was to
> > call mips_classify_address if TARGET_MIPS16 is set because it will do a
> > more complete mips16 addressing check and reject operands that do not meet
> > the more restrictive requirements.
> >
> > I ran the GCC testsuite with no regressions and have included a test case as
> > part of this patch.
> >
> > OK to checkin?
> >
> > Steve Ellcey
> > sellcey@imgtec.com
> >
> >
> > 2016-01-26  Steve Ellcey  <sellcey@imgtec.com>
> >
> > 	PR target/68400
> > 	* config/mips/mips.c (and_operands_ok): Add MIPS16 check.
> >
> >
> >
> > diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
> > index dd54d6a..adeafa3 100644
> > --- a/gcc/config/mips/mips.c
> > +++ b/gcc/config/mips/mips.c
> > @@ -8006,9 +8006,18 @@ mask_low_and_shift_p (machine_mode mode, rtx mask, rtx shift, int
> maxlen)
> >  bool
> >  and_operands_ok (machine_mode mode, rtx op1, rtx op2)
> >  {
> > -  return (memory_operand (op1, mode)
> > -	  ? and_load_operand (op2, mode)
> > -	  : and_reg_operand (op2, mode));
> > +
> > +  if (memory_operand (op1, mode))
> > +    {
> > +      if (TARGET_MIPS16) {
> > +	struct mips_address_info addr;
> > +	if (!mips_classify_address (&addr, op1, mode, false))
> > +	  return false;
> > +      }
> 
> Nit: brace formatting.
> 
> It looks like the patch only works by accident.  The code above
> is passing the MEM, rather than the address inside the MEM, to
> mips_classify_address.  Since (mem (mem ...)) is never valid on MIPS,
> the effect is to disable the memory alternatives of *and<mode>3_mips16
> unconditionally.
> 
> The addresses that occur in the testcase are valid as far as
> mips_classify_address is concerned.  FWIW, there shouldn't be any
> difference between the addresses that memory_operand accepts and the
> addresses that mips_classify_address accepts.
> 
> In theory, the "W" constraints in *and<mode>3_mips16 are supposed to
> tell the target-independent code that this instruction cannot handle
> constant addresses or stack-based addresses.  That seems to be working
> correctly during RA for the testcase.  The problem comes in regcprop,
> which ends up creating a second stack pointer rtx distinct from
> stack_pointer_rtx:
> 
>     (reg/f:SI 29 $sp [375])
> 
> (Note the ORIGINAL_REGNO of 375.)  This then defeats the test in
> mips_stack_address_p:
> 
> bool
> mips_stack_address_p (rtx x, machine_mode mode)
> {
>   struct mips_address_info addr;
> 
>   return (mips_classify_address (&addr, x, mode, false)
> 	  && addr.type == ADDRESS_REG
> 	  && addr.reg == stack_pointer_rtx);
> }
> 
> Change the == to rtx_equal_p and the test passes.  I don't think that's
> the correct fix though -- the fix is to stop a second stack pointer rtx
> from being created.

Agreed, though I'm inclined to say do both. We actually hit this
same issue while testing some 4.9.2 based tools recently but I hadn't
got confirmation from Andrew (cc'd) whether it was definitely the same
issue. Andrew fixed this by updating all tests against stack_pointer_rtx
to compare register numbers instead (but rtx_equal_p is better still).

I think it is worthwhile looking in more detail why a new stack pointer
rtx appears. Andrew did look briefly at the time but I don't recall his
conclusions...

Matthew

RE: [Patch, MIPS] Patch for PR 68400, a mips16 bug

[Andrew Bennett]

> -----Original Message-----
> From: Matthew Fortune
> Sent: 30 January 2016 16:46
> To: Richard Sandiford; Steve Ellcey
> Cc: gcc-patches@gcc.gnu.org; clm@codesourcery.com; Andrew Bennett
> Subject: RE: [Patch, MIPS] Patch for PR 68400, a mips16 bug
> 
> Richard Sandiford <rdsandiford@googlemail.com> writes:
> > "Steve Ellcey " <sellcey@imgtec.com> writes:
> > > Here is a patch for PR6400.  The problem is that and_operands_ok was
> checking
> > > one operand to see if it was a memory_operand but MIPS16 addressing is
> more
> > > restrictive than what the general memory_operand allows.  The fix was to
> > > call mips_classify_address if TARGET_MIPS16 is set because it will do a
> > > more complete mips16 addressing check and reject operands that do not meet
> > > the more restrictive requirements.
> > >
> > > I ran the GCC testsuite with no regressions and have included a test case
> as
> > > part of this patch.
> > >
> > > OK to checkin?
> > >
> > > Steve Ellcey
> > > sellcey@imgtec.com
> > >
> > >
> > > 2016-01-26  Steve Ellcey  <sellcey@imgtec.com>
> > >
> > > 	PR target/68400
> > > 	* config/mips/mips.c (and_operands_ok): Add MIPS16 check.
> > >
> > >
> > >
> > > diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
> > > index dd54d6a..adeafa3 100644
> > > --- a/gcc/config/mips/mips.c
> > > +++ b/gcc/config/mips/mips.c
> > > @@ -8006,9 +8006,18 @@ mask_low_and_shift_p (machine_mode mode, rtx mask,
> rtx shift, int
> > maxlen)
> > >  bool
> > >  and_operands_ok (machine_mode mode, rtx op1, rtx op2)
> > >  {
> > > -  return (memory_operand (op1, mode)
> > > -	  ? and_load_operand (op2, mode)
> > > -	  : and_reg_operand (op2, mode));
> > > +
> > > +  if (memory_operand (op1, mode))
> > > +    {
> > > +      if (TARGET_MIPS16) {
> > > +	struct mips_address_info addr;
> > > +	if (!mips_classify_address (&addr, op1, mode, false))
> > > +	  return false;
> > > +      }
> >
> > Nit: brace formatting.
> >
> > It looks like the patch only works by accident.  The code above
> > is passing the MEM, rather than the address inside the MEM, to
> > mips_classify_address.  Since (mem (mem ...)) is never valid on MIPS,
> > the effect is to disable the memory alternatives of *and<mode>3_mips16
> > unconditionally.
> >
> > The addresses that occur in the testcase are valid as far as
> > mips_classify_address is concerned.  FWIW, there shouldn't be any
> > difference between the addresses that memory_operand accepts and the
> > addresses that mips_classify_address accepts.
> >
> > In theory, the "W" constraints in *and<mode>3_mips16 are supposed to
> > tell the target-independent code that this instruction cannot handle
> > constant addresses or stack-based addresses.  That seems to be working
> > correctly during RA for the testcase.  The problem comes in regcprop,
> > which ends up creating a second stack pointer rtx distinct from
> > stack_pointer_rtx:
> >
> >     (reg/f:SI 29 $sp [375])
> >
> > (Note the ORIGINAL_REGNO of 375.)  This then defeats the test in
> > mips_stack_address_p:
> >
> > bool
> > mips_stack_address_p (rtx x, machine_mode mode)
> > {
> >   struct mips_address_info addr;
> >
> >   return (mips_classify_address (&addr, x, mode, false)
> > 	  && addr.type == ADDRESS_REG
> > 	  && addr.reg == stack_pointer_rtx);
> > }
> >
> > Change the == to rtx_equal_p and the test passes.  I don't think that's
> > the correct fix though -- the fix is to stop a second stack pointer rtx
> > from being created.
> 
> Agreed, though I'm inclined to say do both. We actually hit this
> same issue while testing some 4.9.2 based tools recently but I hadn't
> got confirmation from Andrew (cc'd) whether it was definitely the same
> issue. Andrew fixed this by updating all tests against stack_pointer_rtx
> to compare register numbers instead (but rtx_equal_p is better still).
> 
> I think it is worthwhile looking in more detail why a new stack pointer
> rtx appears. Andrew did look briefly at the time but I don't recall his
> conclusions...

I will do some digging to find the testcase that caused the issue on the 4.9.2 tools.  
In the meantime below is the initial patch I created to fix this issue.  I am currently 
in the middle of preparing it for submission and it should be on the mailing list in 
the next few days.  I will take the rtx_equal_p comment on board and rework the code 
to use this function rather than doing a register comparison.


Regards,



Andrew


diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index dd54d6a..1c49f55 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -2435,7 +2435,7 @@ mips_stack_address_p (rtx x, machine_mode mode)
 
   return (mips_classify_address (&addr, x, mode, false)
 	  && addr.type == ADDRESS_REG
-	  && addr.reg == stack_pointer_rtx);
+	  && REGNO (addr.reg) == STACK_POINTER_REGNUM);
 }
 
 /* Return true if ADDR matches the pattern for the LWXS load scaled indexed
@@ -2498,7 +2498,8 @@ mips16_unextended_reference_p (machine_mode mode, rtx base,
 {
   if (mode != BLKmode && offset % GET_MODE_SIZE (mode) == 0)
     {
-      if (GET_MODE_SIZE (mode) == 4 && base == stack_pointer_rtx)
+      if (GET_MODE_SIZE (mode) == 4 && GET_CODE (base) == REG
+          && REGNO (base) == STACK_POINTER_REGNUM)
 	return offset < 256U * GET_MODE_SIZE (mode);
       return offset < 32U * GET_MODE_SIZE (mode);
     }
@@ -8822,7 +8823,7 @@ mips_debugger_offset (rtx addr, HOST_WIDE_INT offset)
   if (offset == 0)
     offset = INTVAL (offset2);
 
-  if (reg == stack_pointer_rtx
+  if ((GET_CODE (reg) == REG && REGNO (reg) == STACK_POINTER_REGNUM)
       || reg == frame_pointer_rtx
       || reg == hard_frame_pointer_rtx)
     {
@@ -9489,7 +9490,7 @@ mips16e_collect_argument_save_p (rtx dest, rtx src, rtx *reg_values,
   required_offset = cfun->machine->frame.total_size + argno * UNITS_PER_WORD;
   if (base == hard_frame_pointer_rtx)
     required_offset -= cfun->machine->frame.hard_frame_pointer_offset;
-  else if (base != stack_pointer_rtx)
+  else if (!(GET_CODE (base) == REG && REGNO (base) == STACK_POINTER_REGNUM))
     return false;
   if (offset != required_offset)
     return false;
@@ -9700,7 +9701,7 @@ mips16e_save_restore_pattern_p (rtx pattern, HOST_WIDE_INT adjust,
       /* Check that the address is the sum of the stack pointer and a
 	 possibly-zero constant offset.  */
       mips_split_plus (XEXP (mem, 0), &base, &offset);
-      if (base != stack_pointer_rtx)
+      if (!(GET_CODE (base) == REG && REGNO (base) == STACK_POINTER_REGNUM))
 	return false;
 
       /* Check that SET's other operand is a register.  */
@@ -11826,7 +11827,8 @@ mips_restore_reg (rtx reg, rtx mem)
 static void
 mips_deallocate_stack (rtx base, rtx offset, HOST_WIDE_INT new_frame_size)
 {
-  if (base == stack_pointer_rtx && offset == const0_rtx)
+  if (GET_CODE (base) == REG && REGNO (base) == STACK_POINTER_REGNUM
+      && offset == const0_rtx)
     return;
 
   mips_frame_barrier ();
@@ -15652,7 +15654,7 @@ r10k_simplify_address (rtx x, rtx_insn *insn)
 	    {
 	      /* Replace the incoming value of $sp with
 		 virtual_incoming_args_rtx.  */
-	      if (x == stack_pointer_rtx
+	      if (GET_CODE (x) == REG && REGNO (x) == STACK_POINTER_REGNUM
 		  && DF_REF_BB (def) == ENTRY_BLOCK_PTR_FOR_FN (cfun))
 		newx = virtual_incoming_args_rtx;
 	    }
diff --git a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md
index 188308a..2384bb6 100644
--- a/gcc/config/mips/mips.md
+++ b/gcc/config/mips/mips.md
@@ -7338,7 +7338,7 @@ (define_insn "*mips16e_save_restore"
        [(set (match_operand:SI 1 "register_operand")
 	     (plus:SI (match_dup 1)
 		      (match_operand:SI 2 "const_int_operand")))])]
-  "operands[1] == stack_pointer_rtx
+  "GET_CODE (operands[1]) == REG && REGNO (operands[1]) == STACK_POINTER_REGNUM
    && mips16e_save_restore_pattern_p (operands[0], INTVAL (operands[2]), NULL)"
   { return mips16e_output_save_restore (operands[0], INTVAL (operands[2])); }
   [(set_attr "type" "arith")

Re: [Patch, MIPS] Patch for PR 68400, a mips16 bug

[Richard Sandiford]

Andrew Bennett <Andrew.Bennett@imgtec.com> writes:
>> -----Original Message-----
>> From: Matthew Fortune
>> Sent: 30 January 2016 16:46
>> To: Richard Sandiford; Steve Ellcey
>> Cc: gcc-patches@gcc.gnu.org; clm@codesourcery.com; Andrew Bennett
>> Subject: RE: [Patch, MIPS] Patch for PR 68400, a mips16 bug
>> 
>> Richard Sandiford <rdsandiford@googlemail.com> writes:
>> > "Steve Ellcey " <sellcey@imgtec.com> writes:
>> > > Here is a patch for PR6400.  The problem is that and_operands_ok was
>> checking
>> > > one operand to see if it was a memory_operand but MIPS16 addressing is
>> more
>> > > restrictive than what the general memory_operand allows.  The fix was to
>> > > call mips_classify_address if TARGET_MIPS16 is set because it will do a
>> > > more complete mips16 addressing check and reject operands that do not meet
>> > > the more restrictive requirements.
>> > >
>> > > I ran the GCC testsuite with no regressions and have included a test case
>> as
>> > > part of this patch.
>> > >
>> > > OK to checkin?
>> > >
>> > > Steve Ellcey
>> > > sellcey@imgtec.com
>> > >
>> > >
>> > > 2016-01-26  Steve Ellcey  <sellcey@imgtec.com>
>> > >
>> > > 	PR target/68400
>> > > 	* config/mips/mips.c (and_operands_ok): Add MIPS16 check.
>> > >
>> > >
>> > >
>> > > diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
>> > > index dd54d6a..adeafa3 100644
>> > > --- a/gcc/config/mips/mips.c
>> > > +++ b/gcc/config/mips/mips.c
>> > > @@ -8006,9 +8006,18 @@ mask_low_and_shift_p (machine_mode mode, rtx mask,
>> rtx shift, int
>> > maxlen)
>> > >  bool
>> > >  and_operands_ok (machine_mode mode, rtx op1, rtx op2)
>> > >  {
>> > > -  return (memory_operand (op1, mode)
>> > > -	  ? and_load_operand (op2, mode)
>> > > -	  : and_reg_operand (op2, mode));
>> > > +
>> > > +  if (memory_operand (op1, mode))
>> > > +    {
>> > > +      if (TARGET_MIPS16) {
>> > > +	struct mips_address_info addr;
>> > > +	if (!mips_classify_address (&addr, op1, mode, false))
>> > > +	  return false;
>> > > +      }
>> >
>> > Nit: brace formatting.
>> >
>> > It looks like the patch only works by accident.  The code above
>> > is passing the MEM, rather than the address inside the MEM, to
>> > mips_classify_address.  Since (mem (mem ...)) is never valid on MIPS,
>> > the effect is to disable the memory alternatives of *and<mode>3_mips16
>> > unconditionally.
>> >
>> > The addresses that occur in the testcase are valid as far as
>> > mips_classify_address is concerned.  FWIW, there shouldn't be any
>> > difference between the addresses that memory_operand accepts and the
>> > addresses that mips_classify_address accepts.
>> >
>> > In theory, the "W" constraints in *and<mode>3_mips16 are supposed to
>> > tell the target-independent code that this instruction cannot handle
>> > constant addresses or stack-based addresses.  That seems to be working
>> > correctly during RA for the testcase.  The problem comes in regcprop,
>> > which ends up creating a second stack pointer rtx distinct from
>> > stack_pointer_rtx:
>> >
>> >     (reg/f:SI 29 $sp [375])
>> >
>> > (Note the ORIGINAL_REGNO of 375.)  This then defeats the test in
>> > mips_stack_address_p:
>> >
>> > bool
>> > mips_stack_address_p (rtx x, machine_mode mode)
>> > {
>> >   struct mips_address_info addr;
>> >
>> >   return (mips_classify_address (&addr, x, mode, false)
>> > 	  && addr.type == ADDRESS_REG
>> > 	  && addr.reg == stack_pointer_rtx);
>> > }
>> >
>> > Change the == to rtx_equal_p and the test passes.  I don't think that's
>> > the correct fix though -- the fix is to stop a second stack pointer rtx
>> > from being created.
>> 
>> Agreed, though I'm inclined to say do both. We actually hit this
>> same issue while testing some 4.9.2 based tools recently but I hadn't
>> got confirmation from Andrew (cc'd) whether it was definitely the same
>> issue. Andrew fixed this by updating all tests against stack_pointer_rtx
>> to compare register numbers instead (but rtx_equal_p is better still).

It looks from the patch like it's only "all" for the MIPS target.
Target-independent code would continue to expect pointer equality.

So sorry to be awkward, but I really don't think it's a good idea
to do both.  If we want to allow more than one stack pointer rtx,
we should do it consistently across the codebase rather than in
specific parts of one target.  And if we do that, there's no
need to "fix" the regcprop.c issue; we'd then have redefined
things so that the current regcprop.c behaviour is correct.

If instead we decide to stick with the traditional semantics and
require the stack pointer rtx to be exactly stack_pointer_rtx,
we should just fix the regcprop.c bug and leave the comparisons
with stack_pointer_rtx alone.

Thanks,
Richard

RE: [Patch, MIPS] Patch for PR 68400, a mips16 bug

[Andrew Bennett]

> -----Original Message-----
> From: Richard Sandiford [mailto:rdsandiford@googlemail.com]
> Sent: 03 February 2016 22:45
> To: Andrew Bennett
> Cc: Matthew Fortune; Steve Ellcey; gcc-patches@gcc.gnu.org;
> clm@codesourcery.com
> Subject: Re: [Patch, MIPS] Patch for PR 68400, a mips16 bug
> 
> Andrew Bennett <Andrew.Bennett@imgtec.com> writes:
> >> -----Original Message-----
> >> From: Matthew Fortune
> >> Sent: 30 January 2016 16:46
> >> To: Richard Sandiford; Steve Ellcey
> >> Cc: gcc-patches@gcc.gnu.org; clm@codesourcery.com; Andrew Bennett
> >> Subject: RE: [Patch, MIPS] Patch for PR 68400, a mips16 bug
> >>
> >> Richard Sandiford <rdsandiford@googlemail.com> writes:
> >> > "Steve Ellcey " <sellcey@imgtec.com> writes:
> >> > > Here is a patch for PR6400.  The problem is that and_operands_ok was
> >> checking
> >> > > one operand to see if it was a memory_operand but MIPS16 addressing is
> >> more
> >> > > restrictive than what the general memory_operand allows.  The fix was
> to
> >> > > call mips_classify_address if TARGET_MIPS16 is set because it will do a
> >> > > more complete mips16 addressing check and reject operands that do not
> meet
> >> > > the more restrictive requirements.
> >> > >
> >> > > I ran the GCC testsuite with no regressions and have included a test
> case
> >> as
> >> > > part of this patch.
> >> > >
> >> > > OK to checkin?
> >> > >
> >> > > Steve Ellcey
> >> > > sellcey@imgtec.com
> >> > >
> >> > >
> >> > > 2016-01-26  Steve Ellcey  <sellcey@imgtec.com>
> >> > >
> >> > > 	PR target/68400
> >> > > 	* config/mips/mips.c (and_operands_ok): Add MIPS16 check.
> >> > >
> >> > >
> >> > >
> >> > > diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
> >> > > index dd54d6a..adeafa3 100644
> >> > > --- a/gcc/config/mips/mips.c
> >> > > +++ b/gcc/config/mips/mips.c
> >> > > @@ -8006,9 +8006,18 @@ mask_low_and_shift_p (machine_mode mode, rtx
> mask,
> >> rtx shift, int
> >> > maxlen)
> >> > >  bool
> >> > >  and_operands_ok (machine_mode mode, rtx op1, rtx op2)
> >> > >  {
> >> > > -  return (memory_operand (op1, mode)
> >> > > -	  ? and_load_operand (op2, mode)
> >> > > -	  : and_reg_operand (op2, mode));
> >> > > +
> >> > > +  if (memory_operand (op1, mode))
> >> > > +    {
> >> > > +      if (TARGET_MIPS16) {
> >> > > +	struct mips_address_info addr;
> >> > > +	if (!mips_classify_address (&addr, op1, mode, false))
> >> > > +	  return false;
> >> > > +      }
> >> >
> >> > Nit: brace formatting.
> >> >
> >> > It looks like the patch only works by accident.  The code above
> >> > is passing the MEM, rather than the address inside the MEM, to
> >> > mips_classify_address.  Since (mem (mem ...)) is never valid on MIPS,
> >> > the effect is to disable the memory alternatives of *and<mode>3_mips16
> >> > unconditionally.
> >> >
> >> > The addresses that occur in the testcase are valid as far as
> >> > mips_classify_address is concerned.  FWIW, there shouldn't be any
> >> > difference between the addresses that memory_operand accepts and the
> >> > addresses that mips_classify_address accepts.
> >> >
> >> > In theory, the "W" constraints in *and<mode>3_mips16 are supposed to
> >> > tell the target-independent code that this instruction cannot handle
> >> > constant addresses or stack-based addresses.  That seems to be working
> >> > correctly during RA for the testcase.  The problem comes in regcprop,
> >> > which ends up creating a second stack pointer rtx distinct from
> >> > stack_pointer_rtx:
> >> >
> >> >     (reg/f:SI 29 $sp [375])
> >> >
> >> > (Note the ORIGINAL_REGNO of 375.)  This then defeats the test in
> >> > mips_stack_address_p:
> >> >
> >> > bool
> >> > mips_stack_address_p (rtx x, machine_mode mode)
> >> > {
> >> >   struct mips_address_info addr;
> >> >
> >> >   return (mips_classify_address (&addr, x, mode, false)
> >> > 	  && addr.type == ADDRESS_REG
> >> > 	  && addr.reg == stack_pointer_rtx);
> >> > }
> >> >
> >> > Change the == to rtx_equal_p and the test passes.  I don't think that's
> >> > the correct fix though -- the fix is to stop a second stack pointer rtx
> >> > from being created.
> >>
> >> Agreed, though I'm inclined to say do both. We actually hit this
> >> same issue while testing some 4.9.2 based tools recently but I hadn't
> >> got confirmation from Andrew (cc'd) whether it was definitely the same
> >> issue. Andrew fixed this by updating all tests against stack_pointer_rtx
> >> to compare register numbers instead (but rtx_equal_p is better still).
> 
> It looks from the patch like it's only "all" for the MIPS target.
> Target-independent code would continue to expect pointer equality.
> 
> So sorry to be awkward, but I really don't think it's a good idea
> to do both.  If we want to allow more than one stack pointer rtx,
> we should do it consistently across the codebase rather than in
> specific parts of one target.  And if we do that, there's no
> need to "fix" the regcprop.c issue; we'd then have redefined
> things so that the current regcprop.c behaviour is correct.
> 
> If instead we decide to stick with the traditional semantics and
> require the stack pointer rtx to be exactly stack_pointer_rtx,
> we should just fix the regcprop.c bug and leave the comparisons
> with stack_pointer_rtx alone.

Hi Richard,

I can confirm that the testcase that was failing on the 4.9.2 toolchain
was gcc.dg/torture/pr52028.c, and it was failing in the same manner as
the testcase described here.

Secondly, I think the best approach is to fix the issue within regcprop.c.
If target-independent passes are expecting stack pointer equality then
if the regcprop pass is modifying the stack pointer rtx it could cause 
later passes to generate invalid code.


Regards,



Andrew