diff mbox

Fix newlib build failure for mips16

Message ID 045ca256-da1d-56a0-f97d-0063b1ef4b36@redhat.com
State New
Headers show

Commit Message

Jeff Law April 14, 2017, 5:30 p.m. UTC 
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 mbox

Patch

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))