Kyrill Tkachov <kyrylo.tkac...@arm.com> writes:
> On 03/05/14 20:21, Richard Sandiford wrote:
>> 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. :-))
>
> Hi all,
> This caused some testsuite failures on arm:
> FAIL: gcc.target/arm/vfp-ldmdbd.c scan-assembler fldmdbd
> FAIL: gcc.target/arm/vfp-ldmdbs.c scan-assembler fldmdbs
> FAIL: gcc.target/arm/vfp-ldmiad.c scan-assembler fldmiad
> FAIL: gcc.target/arm/vfp-ldmias.c scan-assembler fldmias
>
> From the vfp-ldmdbd.c test this patch changed the codegen from:
> fldmdbd r5!, {d7}
>
> into
> sub r5, r5, #8
> fldd d7, [r5]
>
> Which broke the test.
Sorry for the breakage. I've reverted the patch for now and will send a
fixed version when I have time.
Thanks,
Richard
