[PATCH 2/3] Add XLP-specific atomic instructions and tweaks.

[Richard Sandiford]

Maxim Kuvyrkov <maxim@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