diff mbox

Fix newlib build failure for mips16

Message ID 8ae83192-929a-a677-639e-100e036672a0@redhat.com
State New
Headers show

Commit Message

Jeff Law April 14, 2017, 5:10 a.m. UTC 
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.

Sadly, we don't have a good predicate to use for the source operand of 
an extendsidi pattern.  In general sp+offset is a valid memory address, 
but not for lwu.

I briefly pondered disabling the pattern for mips16, but that seems 
somewhat anti-performant.  So I looked at alternatives.

I poked a bit at adding an appropriate operand predicate, but it just 
gets ugly as to do it right.  I'd want to use some of the 
GO_IF_LEGITIMATE_ADDRESS machinery to decompose the address.  But 
everything is static.  Exposing it would be possible I suppose.

I could also have passed in a flag to the GO_IF_LEGITIMATE_ADDRESS 
machinery to indicating we're handling lwu, but that seemed hacky.

Instead I added additional tests to the pattern's condition to verify 
that if TARGET_MIPS16 was active, the input operand was not a MEM where 
sp appears in the address.  That's fairly surgical so we're not going to 
adversely affect code generation and doesn't require hacking up the 
GO_IF_LEGITIMATE_ADDRESS machinery.

That allows mips64vr-elf to build newlib & libgcc.  It was certainly a 
regression as we've been able to build mips16 newlib multilibs in the past.

Installing on the trunk.

Jeff


ps.  bz74563 (P2 regression) is an unrelated mips16 issue introduced a 
couple years ago that probably makes classic mips16 unusable right now. 
I may take a stab at fixing that too since I have a reasonable idea 
what's happening.

Comments

Richard Sandiford April 14, 2017, 11:22 a.m. UTC | #1 
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.

Adding code to do that to individual MIPS patterns feels like a hack to me.
The pass should be doing it itself.

Thanks,
Richard
Jeff Law April 14, 2017, 4:12 p.m. UTC | #2 
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.


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

jeff
Richard Sandiford April 14, 2017, 4:24 p.m. UTC | #3 
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.

Thanks,
Richard
Jeff Law April 14, 2017, 5:01 p.m. UTC | #4 
On 04/14/2017 10:24 AM, Richard Sandiford wrote:
>
> 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.
Hmm, you've of course right!


>
>>> 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.
Let me dig a little further.  Two stack pointers just sounds 
fundamentally wrong.

jeff
Jeff Law April 14, 2017, 5:09 p.m. UTC | #5 
On 04/14/2017 11:01 AM, Jeff Law wrote:
> On 04/14/2017 10:24 AM, Richard Sandiford wrote:
>>
>> 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.
> Hmm, you've of course right!
>
>
>>
>>>> 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.
> Let me dig a little further.  Two stack pointers just sounds
> fundamentally wrong.
See maybe_mode_change and cry.

jeff
diff mbox

Patch

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 33b094e..788f029 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,9 @@ 
+2017-04-13  Jeff Law  <law@redhat.com>
+
+	* config/mips.mips.md (zero_extendsidi2): Do not allow SP to appear
+	in operands[1] if it is a MEM and TARGET_MIPS16 is active.
+	(zero_extendsidi2_dext): Likewise.
+
 2017-04-13  Jakub Jelinek  <jakub@redhat.com>
 
 	PR sanitizer/80403
diff --git a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md
index 7acf00d..dd5e1e7 100644
--- a/gcc/config/mips/mips.md
+++ b/gcc/config/mips/mips.md
@@ -3493,7 +3493,10 @@ 
 (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_64BIT && !ISA_HAS_EXT_INS
+   && !(TARGET_MIPS16
+        && MEM_P (operands[1])
+        && reg_mentioned_p (stack_pointer_rtx, operands[1]))"
   "@
    #
    lwu\t%0,%1"
@@ -3509,7 +3512,10 @@ 
 (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_64BIT && ISA_HAS_EXT_INS
+   && !(TARGET_MIPS16
+        && MEM_P (operands[1])
+        && reg_mentioned_p (stack_pointer_rtx, operands[1]))"
   "@
    dext\t%0,%1,0,32
    lwu\t%0,%1"