This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
RE: [Patch, MIPS] Patch for PR 68400, a mips16 bug
- From: Andrew Bennett <Andrew dot Bennett at imgtec dot com>
- To: Matthew Fortune <Matthew dot Fortune at imgtec dot com>, Richard Sandiford <rdsandiford at googlemail dot com>, Steve Ellcey <Steve dot Ellcey at imgtec dot com>
- Cc: "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>, "clm at codesourcery dot com" <clm at codesourcery dot com>
- Date: Mon, 1 Feb 2016 13:40:22 +0000
- Subject: RE: [Patch, MIPS] Patch for PR 68400, a mips16 bug
- Authentication-results: sourceware.org; auth=none
- References: <1d526ceb-716b-4c64-bf3b-bb9a1222d0aa at BAMAIL02 dot ba dot imgtec dot org> <874mdvw4n0 dot fsf at googlemail dot com> <6D39441BF12EF246A7ABCE6654B023536A733D61 at LEMAIL01 dot le dot imgtec dot org>
> -----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")