On 04/14/2017 10:24 AM, Richard Sandiford wrote:
Jeff Law <l...@redhat.com> writes:
On 04/14/2017 05:22 AM, Richard Sandiford wrote:
Jeff Law <l...@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))