Re: [Patch, MIPS] Patch for PR 68400, a mips16 bug

[Richard Sandiford <rdsandiford at googlemail dot com>]

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

Thanks,
Richard