Re: RFC: [PATCH] MIPS: Implement built-in atomic memory operations.

[Richard Sandiford <richard at codesourcery dot com>]

David Daney <ddaney@avtrex.com> writes:
> Richard Sandiford wrote:
>> So I'd suggest having one pattern each for the loops in:
>>
>>   - sync_compare_and_swap<mode>
>>   - sync_old_<fetch_op><mode>
>>   - sync_old_nand<mode>
>>   - sync_new_<fetch_op><mode>
>>   - sync_new_nand<mode>
>>
>>   
> Like this?
>
> I split the fetch_ops into two groups (arithmetic and bitwise).  This 
> was to make it possible to have patterns that matched immediate operands 
> as they are treated differently in the two cases (sign extended vs. zero 
> extended).

FWIW, that difference could be handled by code attributes.  I think the
real problem is the need for "<d>" in arithmetic ops and not logical ops.

> +(define_insn "sync_compare_and_swap<mode>"
> +  [(set (match_operand:GPR 0 "register_operand" "=&d,d")
> +	(match_operand:GPR 1 "memory_operand" "+R,R"))
> +   (set (match_dup 1)
> +	(unspec_volatile:GPR [(match_operand:GPR 2 "register_operand" "d,d")
> +			      (match_operand:GPR 3 "arith_operand" "I,d")]
> +	 UNSPEC_COMPARE_AND_SWAP))
> +   (clobber (match_scratch:GPR 4 "=d,d"))]

Operand 4 needs to be earlyclobber too, to handle the case where the
loop iterates.  But why not use $at instead?

FWIW, we could handle "IKL" (with an appropriate predicate)
without any change to the template.

> +{
> +  output_asm_insn(".set\tpush\n"
> +         "\t.set\tnoreorder\n"
> +         "\t.set\tnomacro\n"
> +	 "\tsync\n"
> +	 "1:\tll<d>\t%0,%1\n"
> +	 "\tbne\t%0,%2,2f", operands);
> +
> +  if (which_alternative == 0)
> +    output_asm_insn("li\t%4,%3", operands);
> +  else
> +    output_asm_insn("move\t%4,%3", operands);
> +
> +  return "sc<d>\t%4,%1\n"
> +	 "\tbeq\t%4,$0,1b\n"
> +	 "\tnop\n"
> +	 "2:\n"
> +         "\t.set\tpop";
> +}
> +  [(set_attr "length" "28")])

Please use %(%<...%>%) to wrap the set noreorder/set nomacro stuff.
Make that %(%<%[...%]%>%) if we're using $at (nice and readable, eh?)
Also use $@ instead of $at and $. instead of $0.

I think it would be (slightly) cleaner to have:

/* Return an asm string that atomically:

     - Compares memory reference %1 to register %2 and, if they are
       equal, changes %1 to %3.

     - Sets register %0 to the old value of memory reference %1.

   SUFFIX is the suffix that should be added to "ll" and "sc" instructions
   and OP is the instruction that should be used to load %3 into a
   register.  */
#define MIPS_COMPARE_AND_SWAP(SUFFIX, OP)	\
  "%(%<%[sync\n"				\
  "1:\tll" SUFFIX "t%0,%1\n"			\
  "\tbne\t%0,%2,2f\n"				\
  OP "\t%@,%3\n"				\
  "\tsd" SUFFIX "%@,%1\n"			\
  "\tbeq\t%@,%.,1b\n"				\
  "\tnop\n"					\
  "2:%]%>%)\n"
....
  if (which_alternative == 0)
    return MIPS_COMPARE_AND_SWAP ("<d>", "li");
  else
    return MIPS_COMPARE_AND_SWAP ("<d>", "move");

The win isn't that big here, but it's bigger if we do the same thing
for the (repeated) sync_old* and sync_new* patterns; more below.

> +(define_code_macro FETCHOP_ARITH [plus minus])

These are usually lowercase for MIPS.  Please put at the top of the
file, with the others.

> +(define_code_attr fetchop_arith_name [(plus "add") (minus "sub")])

Add this to the "optab" attribute

> +(define_code_attr fetchop_arith_op [(plus "addu") (minus "subu")])

...and this to the "insn" attribute.

> +(define_code_attr fetchop_arith_imm_op [(plus "addiu") (minus "subiu")])

We shouldn't pretend there's a "subiu" insn.  I suppose this is where
the idea of using code macros falls down ;(  We could get around it by
adding code attributes for the predicate and constraint (as Alpha does)
but perhaps it is better to keep them separate.

> +(define_insn "sync_old_<fetchop_arith_name><mode>"
> +  [(set (match_operand:GPR 0 "register_operand" "=&d,d")
> +	(match_operand:GPR 1 "memory_operand" "+R,R"))
> +   (set (match_dup 1)
> +	(unspec_volatile:GPR
> +          [(FETCHOP_ARITH:GPR (match_operand:GPR 2 "arith_operand" "I,d")
> +			      (match_dup 2))]
> +	 UNSPEC_SYNC_OLD_OP))
> +   (clobber (match_scratch:GPR 3 "=d,d"))]
> +  "ISA_HAS_LL_SC"

Same $at comment here.  Also, the first operand to the fetchop ought to
be 1 rather than 2.  (Not that it makes any difference to the generated
code; it's just a little confusing as-is.)

Again, I think it would be good to add a MIPS_SYNC_OLD macro,
along the lines of MIPS_COMPARE_AND_SWAP above.

> +(define_code_attr fetchop_bit_imm_op [(ior "ori") (xor "xori") (and "andi")])

Let's make this "imm_insn", to match "insn".

> +(define_insn "sync_new_<fetchop_arith_name><mode>"
> +  [(set (match_operand:GPR 0 "register_operand" "=&d,d")
> +	(match_operand:GPR 1 "memory_operand" "+R,R"))
> +   (set (match_dup 1)
> +	(unspec_volatile:GPR
> +          [(FETCHOP_ARITH:GPR (match_operand:GPR 2 "arith_operand" "I,d")
> +			      (match_dup 2))]
> +	 UNSPEC_SYNC_NEW_OP))
> +   (clobber (match_scratch:GPR 3 "=d,d"))]
> +  "ISA_HAS_LL_SC"

Again, this pattern is a little confusing.  Operand 0 is set to the
result of the operation, not the original memory reference.

Same $at comment, and it might be better to have a MIPS_SYNC_NEW macro.

> +(define_insn "sync_new_nand<mode>"
> +  [(set (match_operand:GPR 0 "register_operand" "=&d,d")
> +	(match_operand:GPR 1 "memory_operand" "+R,R"))
> +   (set (match_dup 1)
> +	(unspec_volatile:GPR [(match_operand:GPR 2 "uns_arith_operand" "K,d")]
> +	 UNSPEC_SYNC_NEW_OP))
> +   (clobber (match_scratch:GPR 3 "=d,d"))]
> +  "ISA_HAS_LL_SC"
> +{
> +  output_asm_insn(".set\tpush\n"
> +         "\t.set\tnoreorder\n"
> +         "\t.set\tnomacro\n"
> +	 "\tsync\n"
> +	 "1:\tll<d>\t%0,%1\n"
> +	 "\tnor\t%0,$0,%0", operands);
> +
> +  if (which_alternative == 0)
> +    output_asm_insn("andi\t%3,%0,%2", operands);
> +  else
> +    output_asm_insn("and\t%3,%0,%2", operands);
> +
> +  return "move\t%0,%3\n"
> +	 "\tsc<d>\t%3,%1\n"
> +	 "\tbeq\t%3,$0,1b\n"
> +	 "\tnop\n"
> +         "\t.set\tpop";
> +}

Why not avoid the move and use the same delay-slot repeat that you used
in the other sync_new* patterns?

> +(define_insn "sync_lock_test_and_set<mode>"
> +  [(set (match_operand:GPR 0 "register_operand" "=&d")
> +	(match_operand:GPR 1 "memory_operand" "+R"))
> +   (set (match_dup 1)
> +	(unspec_volatile:GPR [(match_operand:GPR 2 "register_operand" "d")]
> +	 UNSPEC_SYNC_EXCHANGE))
> +   (clobber (match_scratch:GPR 3 "=d"))]
> +  "ISA_HAS_LL_SC"
> +{
> +  return ".set\tpush\n"
> +         "\t.set\tnoreorder\n"
> +         "\t.set\tnomacro\n"
> +	 "1:\tll<d>\t%0,%1\n"
> +	 "\tmove\t%3,%2"
> +	 "\tsc<d>\t%3,%1\n"
> +	 "\tbeq\t%3,$0,1b\n"
> +	 "\tnop\n"
> +	 "\tsync\n"
> +         "\t.set\tpop";
> +}

No need for a C block.  Just make this a plain asm template.

Sorry for all the comments...

Richard