> -----Original Message----- > From: Richard Sandiford [mailto:rdsandif...@googlemail.com] > Sent: 03 February 2016 22:45 > To: Andrew Bennett > Cc: Matthew Fortune; Steve Ellcey; gcc-patches@gcc.gnu.org; > c...@codesourcery.com > Subject: Re: [Patch, MIPS] Patch for PR 68400, a mips16 bug > > Andrew Bennett <andrew.benn...@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; 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). > > 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