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...
Matthew
