> -----Original Message----- > From: Matthew Fortune > Sent: 30 January 2016 16:46 > To: Richard Sandiford; Steve Ellcey > Cc: gcc-patches@gcc.gnu.org; c...@codesourcery.com; Andrew Bennett > Subject: RE: [Patch, MIPS] Patch for PR 68400, a mips16 bug > > Richard Sandiford <rdsandif...@googlemail.com> writes: > > "Steve Ellcey " <sell...@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 > > > sell...@imgtec.com > > > > > > > > > 2016-01-26 Steve Ellcey <sell...@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")