[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