Vladimir Makarov <vmaka...@redhat.com> writes:
>>> Not sure how the constraint would/should exclude $sp-based address in
>>> LRA. In this particular case, a spilled pseudo is changed to memory
>>> giving the following RTL form:
>>>
>>> (insn 30 29 31 4 (set (reg:SI 4 $4)
>>> (and:SI (mem/c:SI (plus:SI (reg/f:SI 78 $frame)
>>> (const_int 16 [0x10])) [7 %sfp+16 S4 A32])
>>> (const_int 65535 [0xffff]))) shell.i:17 161 {*andsi3_mips16}
>>> (expr_list:REG_DEAD (reg:SI 194 [ D.1469 ])
>>> (nil)))
>>>
>>> The operand 1 during alternative selection is not marked as a bad
>>> operand as it is a memory operand. $frame appears to be fine as it
>>> could be eliminated later to hard register. No reloads are inserted
>>> for the instructions concerned. Unless, $frame should be temporarily
>>> eliminated and then a reload would be inserted?
>>
>> Yeah, I think the lack of elimination is the problem. process_address
>> eliminates $frame temporarily before checking whether the address
>> is valid, but the places that check EXTRA_CONSTRAINT_STR pass the
>> original uneliminated address. So the legitimate_address_p hook sees
>> the $sp-based address but the "W" constraint only sees the $frame-based
>> address (which might or might not be valid, depending on whether $frame
>> is eliminated to the stack or hard frame pointer). I think the constraints
>> should see the eliminated address too.
>>
>> This patch seems to fix it for me. Tested on x86_64-linux-gnu.
>> Vlad, is this OK for trunk?
>>
>> BTW, we might want to define something like:
>>
>> #define MODE_BASE_REG_CLASS(MODE) \
>> (TARGET_MIPS16 \
>> ? ((MODE) == SImode || (MODE) == DImode ? M16_SP_REGS : M16_REGS) \
>> : GR_REGS)
>>
>> instead of BASE_REG_CLASS. It might lead to slightly better code
>> (or not -- if it doesn't then don't bother :-)).
>>
>> If this patch is OK then I think the only thing blocking the switch
>> to LRA is the asm-subreg-1.c failure. I think it'd be fine to XFAIL
>> that test on MIPS for now, until there's a consensus about what "X" means
>> for asms.
>>
>>
>> gcc/
>> * lra-constraints.c (valid_address_p): Move earlier in file.
>> Add a constraint argument to the address_info version.
>> (satisfies_memory_constraint_p): New function.
>> (satisfies_address_constraint_p): Likewise.
>> (process_alt_operands, curr_insn_transform): Use them.
>> (process_address): Pass the constraint to valid_address_p when
>> checking address operands.
>>
>>
>
> Yes, it looks ok for me, Richard. Thanks on working on this.
>
> I am on vacation till May 4th. If the patch results in problems on other
> targets, I hope you revert it. But to be honest, I believe it is very
> safe and don't expect any problems at all.
Thanks Vlad, belatedly committed on that basis. Like you say I'll revert
it at the first sign of trouble (although it ended up being closer to
your return than originally intended. :-))
Richard
