[Patch 1/2] MIPS factor sync instructions out of atomic memory built-ins.

[David Daney]

Richard Sandiford wrote:
> Looks good, thanks.  OK to commit with a few minor niggles fixed:
> 

Thanks Richard,

I will post the revised version when it finishes testing.


> David Daney <ddaney@caviumnetworks.com> writes:
>> -    return mips_output_sync_loop (MIPS_COMPARE_AND_SWAP ("<d>", "li"));
>> +    return mips_output_sync_loop (true,
>> +      MIPS_COMPARE_AND_SWAP ("<d>", "li"), operands);
>>    else
>> -    return mips_output_sync_loop (MIPS_COMPARE_AND_SWAP ("<d>", "move"));
>> +    return mips_output_sync_loop (true,
>> +      MIPS_COMPARE_AND_SWAP ("<d>", "move"), operands);
>>  }
>>    [(set_attr "length" "32")])
> 
> Not really canonical formatting.  Maybe we should just split out
> the strings:
> 
>     const char *loop = MIPS_COMPARE_AND_SWAP ("<d>", "li");
>     return mips_output_sync_loop (true, loop, operands);
> 
> I don't mind leaving the length as-is, but it's probably worth
> saying that it's a worst-case length.  Alternatively (and better,
> of course) you could add a new function along the lines of
> mips_idiv_insns.  Please send that for review if you do.
> 

I have plans for another related patch and will add accurate length 
calculation to that set.


>> @@ -135,8 +139,8 @@
>>     (clobber (match_scratch:SI 4 "=&d"))]
>>    "GENERATE_LL_SC"
>>  {
>> -    return (mips_output_sync_loop
>> -	    (MIPS_SYNC_OP_12 ("<insn>", MIPS_SYNC_OP_12_AND)));
>> +    return mips_output_sync_loop (true,
>> +      MIPS_SYNC_OP_12 ("<insn>", MIPS_SYNC_OP_12_AND), operands);
>>  }
>>    [(set_attr "length" "40")])
> 
> Excessive indentation before and after the patch.
> There are a few other cases of this.
> 

Yep.

>> Index: gcc/config/mips/mips.c
>> ===================================================================
>> --- gcc/config/mips/mips.c.orig
>> +++ gcc/config/mips/mips.c
>> @@ -10749,11 +10749,17 @@ mips_output_order_conditional_branch (rt
>>     both normal and likely branches, using %? and %~ where appropriate.  */
>>  
>>  const char *
>> -mips_output_sync_loop (const char *loop)
>> +mips_output_sync_loop (bool barrier_before,
>> +		       const char *loop, rtx *operands)
>>  {
>> +  const char *sync = "sync";
>> +  if (barrier_before)
>> +    output_asm_insn (sync, NULL);
>>    /* Use branch-likely instructions to work around the LL/SC R10000 errata.  */
>>    mips_branch_likely = TARGET_FIX_R10000;
>> -  return loop;
>> +  output_asm_insn (loop, operands);
>> +
>> +  return sync;
> 
> I don't think the sync variable really adds much.  Lets just use
> "sync" directly.  You need to update the comment above the function.
> 

OK.