| Message ID | 0DA23CC379F5F945ACB41CF394B982776A55F307@LEMAIL01.le.imgtec.org |
|---|---|
| State | New |
| Headers | show |
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
> -----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
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")