On 2014-04-21, 8:23 AM, Richard Sandiford wrote:
Robert Suchanek <robert.sucha...@imgtec.com> writes:
Did you see the failures even after your mips_regno_mode_ok_for_base_p
change? LRA should know how to reload a "W" address.
Yes but I realize there is more. It fails because $sp is now included
in BASE_REG_CLASS and "W" is based on it. However, I suppose that
it would be too eager to say it is wrong and likely there is
something missing
in LRA if we want to keep all alternatives. Currently there is no check
if a reloaded operand has a valid address, use of $sp in lbu/lhu cases.
Even if we added extra checks we are less likely to benefit as we need
to reload the base into register.
Not sure what you mean, sorry. "W" exists specifically to exclude
$sp-based and $pc-based addresses. LRA AFAIK should already be able
to reload addresses that are valid in the TARGET_LEGITIMATE_ADDRESS_P
sense but which do not match the constraints for a particular insn.
Can you remember one of the tests that fails?
I couldn't trigger the problem with the original testcase but found
another one that reveals it. The following needs to compiled with
-mips32r2 -mips16 -Os:
struct { int addr; } c;
struct command { int args[1]; };
unsigned short a;
fn1 (struct command *p1)
{
unsigned short d;
d = fn2 ();
a = p1->args[0];
fn3 (a);
if (c.addr)
{
fn4 (p1->args[0]);
return;
}
(&c)->addr = fn5 ();
fn6 (d);
}
Thanks.
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.
