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

[Andrew Bennett <Andrew dot Bennett at imgtec dot com>]

> -----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")