Maxim Kuvyrkov <ma...@codesourcery.com> writes:
> Updated patch attached. Any further comments?
It's due to my bad explanation, sorry, but this isn't what I meant.
The two main changes I was looking for were:
1) Your pattern uses:
[(mem:GPR (match_operand:P 1 "register_operand" "d"))]
Instead, we should define a new memory predicate/constraint pair
for memories that only accept register addresses. I.e. there
should be a new predicate to go alongside things like
memory_operand and stack_operand, except that the new one would
be even more restrictive in the set of addresses that it allows.
mem_reg_operand seems as good a name as any, but I'm not wedded
to a particular name.
The new memory constraint would likewise go alongside "m", "W", etc.,
except that (like the predicate) it too would only allow register
addresses. We're running low on constraint latters, so a two-operand
one like "ZR" might be OK. We can then use "Z" as a prefix for other
MIPS-specific memory and address constraints in future.
The atomic_exchange and atomic_fetch_add expanders should use
the code I quoted in the earlier message to force the original
memory_operand into this more restrictive form:
if (!mem_reg_operand (operands[1], <MODE>mode))
{
addr = force_reg (Pmode, XEXP (operands[1], 0));
operands[1] = replace_equiv_address (operands[1], addr);
}
The reason is that hard-coding (mem ...) in named define_insns
(i.e. those with a gen_* function) is usually a mistake. We end
up discarding the original MEM and losing track of its MEM_ATTRs.
(Note that this change means we don't need separate Pmode == SImode
and Pmode == DImode patterns.)
2) Your pattern has:
(match_operand:GPR 2 "arith_operand" "0")
to match:
(match_operand:GPR 0 "register_operand" "=d")
Operand 2 doesn't accept constants, so it should be a register_operand
rather than an arith_operand. Then the atomic_exchange and atomic_fetch_add
expanders should use force_reg to turn _their_ arith_operands into
register_operands before calling gen_atomic_fetch_add<mode>_ldadd
and gen_atomic_fetch<mode>_swap.
Your new comment says:
/* Spill the address to a register upfront to simplify reload's job. */
But this isn't about making reload's job easier. Reload can cope just
fine with the arith_operand above and would cope just fine with:
(match_operand ... "memory_operand" "ZR")
with ZR defined as above. Instead. we're trying to describe the
instruction as accurately as possible so that the pre-reload passes
(including IRA) are in a position to make good optimisation decisions.
They're less able to do that if patterns claim to accept more things
than they actually do.
I.e. it's the same reason that we don't just use "general_operand"
for all reloadable rvalues and "nonimmediate_operand" for all
reloadable lvalues. Trying to use accurate predicates is such
standard practice that I think it'd be better to drop the comment here.
Having one gives the impression that we're trying to cope with some
special case, which AFAICT we're not.
Thanks,
Richard
