Re: [PATCH] LoongArch: Fix atomic_exchange make comparison and may jump out

[Jinyang He <hejinyang@loongson.cn>]

On 2022/11/15 下午10:21, Xi Ruoyao wrote:

> On Tue, 2022-11-15 at 21:03 +0800, Jinyang He wrote:
>> gcc/ChangeLog:
>>
>> * config/loongarch/sync.md:
>> Add atomic_cas_value_exchange_and_7<mode> and fix atomic_exchange<mode>.
> nit:
>
> 	* config/loongarch/sync.md (atomic_cas_value_exchange_and_7):
> 	New define_insn.
> 	(atomic_exchange): Use atomic_cas_value_exchange_and_7 instead
> 	of atomic_cas_value_cmp_and.
>
>> gcc/testsuite/ChangeLog:
>>
>> * gcc.target/loongarch/sync-1.c: New test.
> Likewise, ChangeLog content should be indented with a tab. (Not 8
> spaces: if my mail client changes my tab to 8 spaces I'm sorry).
>
> /* snip */

OK. Thanks for the clear commit message and the explanation of format.


>> +  return "%G6\\n\\t"
>> +        "1:\\n\\t"
>> +        "ll.<amo>\\t%0,%1\\n\\t"
>> +        "and\\t%7,%0,%z3\\n\\t"
>> +        "or%i5\\t%7,%7,%5\\n\\t"
>> +        "sc.<amo>\\t%7,%1\\n\\t"
>> +        "beqz\\t%7,1b\\n\\t";
> Do we need a "dbar 0x700" after beqz?
>
> /* snip */

That's worth discussing. Actually I don't see any dbar hint definition
like 0x700 in the manual right now.
Besides, I think what should be provided here is a relaxed version. And
whether the barrier exsit or not is depend on the specific memory_order.

https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#_dbar


>
>> diff --git a/gcc/testsuite/gcc.target/loongarch/sync-1.c b/gcc/testsuite/gcc.target/loongarch/sync-1.c
>> new file mode 100644
>> index 000000000..cebed6a9b
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/loongarch/sync-1.c
>> @@ -0,0 +1,104 @@
>> +/* Test __sync_test_and_set in atomic_exchange */
>> +/* { dg-do run } */
>> +/* { dg-options "-lpthread -std=c11" } */
> This test seems not deterministic.  And the use of sched_yield is very
> tricky, as the man page says:
>
>         sched_yield() is intended for use with  real-time  scheduling  policies
>         (i.e., SCHED_FIFO or SCHED_RR).  Use of sched_yield() with nondetermin‐
>         istic scheduling policies such as SCHED_OTHER is unspecified  and  very
>         likely means your application design is broken.

Yes, there might be something wrong. The test is just a variants from
llvm::tsan. It was presented to prove that the old implementation did
have problems.


>
> I'd suggest to create a bug report at https://gcc.gnu.org/bugzilla

Thanks, I need to do that. It is must be I missing something at
https://gcc.gnu.org/contribute.html.


> and
> post this test in the PR.  Then add the PR number into the changelog,
> and just add a { dg-do compile } and { dg-final { scan-assembler ... } }
> test into the testsuite to ensure the correct ll/sc loop is generated.
>
> A bug report also emphasises that this is a bug fix, which is suitable
> for GCC 13 (in stage 3 now) and GCC 12 (the fix will be backported).
>
I will create a bug report where we all can discuss it.
Thanks for your review and help. :-)