On 16/06/2012, at 7:45 PM, Richard Sandiford wrote:
> 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.Richard, Thank you for a thoughtful write-up. I really appreciate the time you are taking to educate me. I've incorporated yours and Richard H.'s comments (stole pieces from ARM port) and attached is the updated patch. The only other change that I made that was not in your comments is the addition of "b" mips_print_operand specifier. The LDADD and SWAP instructions accept their address as a plain register without parenthesis, so I've added the specifier to skip outputting parenthesis. Any further comments? -- Maxim Kuvyrkov CodeSourcery / Mentor Graphics
0002-Add-XLP-specific-atomic-instructions-and-tweaks.patch
Description: Binary data
