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

[Xi Ruoyao <xry111@xry111.site>]

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 */

> +  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 */

> 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.

I'd suggest to create a bug report at https://gcc.gnu.org/bugzilla 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).

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University