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. Thanks, Richard
