Re: [PATCH] Fix newlib build failure for mips16

[Jeff Law <law at redhat dot com>]

On 04/14/2017 10:24 AM, Richard Sandiford wrote:
> Jeff Law <law@redhat.com> writes:
> > On 04/14/2017 05:22 AM, Richard Sandiford wrote:
> > > Jeff Law <law@redhat.com> writes:
> > > > The mips64vr-elf target will fail building newlib, particularly the
> > > > mips16 newlib as we emit bogus assembly code.
> > > >
> > > > In particular the compiler will emit something like
> > > >
> > > > lwu $2,0($sp)
> > > >
> > > > That's invalid in mips16 mode AFAICT.
> > > >
> > > > That's emitted by the extendsidi pattern.  It's a case where the operand
> > > > predicates are looser that the constraints.  The code we get out of
> > > > reload is fine, but hard register propagation substitutes sp for a
> > > > (valid mips16) hard register that as the same value.  Since hard
> > > > register propagation tests predicates, not constraints, the substitution
> > > > is successful and the bogus code is generated.
> > >
> > > Isn't that the bug though?  Post-reload passes must test the constraints
> > > as well as the predicates, to make sure that the change aligns with
> > > one of the available alternatives.
> > I thought that as well and was quite surprised to see regcprop not honor
> > that.
> >
> > regcprop uses the validate_change interface, which normally checks
> > contraints -- except for MEMs which is just checks for validity via
> > memory_address_addr_space_p.  Thus inside a MEM it'll allow any change
> > that is recognized as valid according to the legitimate_address hooks.
> >
> > I would claim that is fundamentally broken, but trying to change that at
> > this stage is, IMHO, unwise.  I think we should seriously consider doing
> > something different for stage1.  For example, after validating the MEM
> > using the legitimate address hooks, call insn_invalid_p as well to
> > verify constraints.
>
> I think it only does that if the "containing object" that you're
> validating is a MEM.  If the object you're validating is an insn
> (which it always is for regcprop) then normal constrain_operands
> does happen.
>
> > > Adding code to do that to individual MIPS patterns feels like a hack to me.
> > > The pass should be doing it itself.
> > Agreed.  It's a hack.  But it was the best I could see to do at this stage.
>
> Been looking at it a bit more, and I think the problem is that we're
> somehow ending up with a second stack pointer rtx, distinct from
> stack_pointer_rtx.  And then I remembered that this had been discussed
> before, see the tail end of:
>
>    https://gcc.gnu.org/ml/gcc-patches/2016-01/msg02362.html
>
> I'd be happier with the mips_stack_address_p change described there,
> although it still seems like a hack.
Here's what I'm going to spin in my testers over the weekend.  It 
reverts my mips.md change and instead rejects making a copy of the stack 
pointer.

Jeff

diff --git a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md
index dd5e1e7..7acf00d 100644
--- a/gcc/config/mips/mips.md
+++ b/gcc/config/mips/mips.md
@@ -3493,10 +3493,7 @@
 (define_insn_and_split "*zero_extendsidi2"
   [(set (match_operand:DI 0 "register_operand" "=d,d")
         (zero_extend:DI (match_operand:SI 1 "nonimmediate_operand" "d,W")))]
-  "TARGET_64BIT && !ISA_HAS_EXT_INS
-   && !(TARGET_MIPS16
-        && MEM_P (operands[1])
-        && reg_mentioned_p (stack_pointer_rtx, operands[1]))"
+  "TARGET_64BIT && !ISA_HAS_EXT_INS"
   "@
    #
    lwu\t%0,%1"
@@ -3512,10 +3509,7 @@
 (define_insn "*zero_extendsidi2_dext"
   [(set (match_operand:DI 0 "register_operand" "=d,d")
         (zero_extend:DI (match_operand:SI 1 "nonimmediate_operand" "d,W")))]
-  "TARGET_64BIT && ISA_HAS_EXT_INS
-   && !(TARGET_MIPS16
-        && MEM_P (operands[1])
-        && reg_mentioned_p (stack_pointer_rtx, operands[1]))"
+  "TARGET_64BIT && ISA_HAS_EXT_INS"
   "@
    dext\t%0,%1,0,32
    lwu\t%0,%1"
diff --git a/gcc/regcprop.c b/gcc/regcprop.c
index ddc6252..367d85a 100644
--- a/gcc/regcprop.c
+++ b/gcc/regcprop.c
@@ -396,6 +396,13 @@ maybe_mode_change (machine_mode orig_mode, machine_mode copy_mode,
       && GET_MODE_SIZE (copy_mode) < GET_MODE_SIZE (new_mode))
     return NULL_RTX;
 
+  /* Avoid creating multiple copies of the stack pointer.  Some ports
+     assume there is one and only one stack pointer.
+
+     It's unclear if we need to do the same for other special registers.  */
+  if (regno == STACK_POINTER_REGNUM)
+    return NULL_RTX;
+
   if (orig_mode == new_mode)
     return gen_raw_REG (new_mode, regno);
   else if (mode_change_ok (orig_mode, new_mode, regno))