From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from loongson.cn (mail.loongson.cn [114.242.206.163]) by sourceware.org (Postfix) with ESMTP id 0F6FD39524BC for ; Wed, 16 Nov 2022 02:12:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0F6FD39524BC Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=loongson.cn Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=loongson.cn Received: from loongson.cn (unknown [111.9.175.10]) by gateway (Coremail) with SMTP id _____8CxLLf3RnRjT40HAA--.10632S3; Wed, 16 Nov 2022 10:12:08 +0800 (CST) Received: from [10.136.12.12] (unknown [111.9.175.10]) by localhost.localdomain (Coremail) with SMTP id AQAAf8Dxd1fyRnRj_UQUAA--.35804S3; Wed, 16 Nov 2022 10:12:04 +0800 (CST) Subject: Re: [PATCH] LoongArch: Fix atomic_exchange make comparison and may jump out To: Xi Ruoyao , Chenghua Xu , Lulu Cheng Cc: Weining Lu , Xing Li , yala , Peng Fan , gcc-patches@gcc.gnu.org References: <20221115130328.15413-1-hejinyang@loongson.cn> <8039c23568889fe85afbe6940ed625448cf6cd56.camel@xry111.site> From: Jinyang He Message-ID: <1dd9ace0-a83f-c530-2d65-5f762e0cc81e@loongson.cn> Date: Wed, 16 Nov 2022 10:11:52 +0800 User-Agent: Mozilla/5.0 (X11; Linux mips64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <8039c23568889fe85afbe6940ed625448cf6cd56.camel@xry111.site> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-CM-TRANSID:AQAAf8Dxd1fyRnRj_UQUAA--.35804S3 X-CM-SenderInfo: pkhmx0p1dqwqxorr0wxvrqhubq/ X-Coremail-Antispam: 1Uk129KBjvJXoWxuF48Ar48Gr1kWFy8XrW3Awb_yoW5Aw15pr W8X3WDKrWkJr93Aan7Wr47JryF9r40k347J34vqa4jvryaqry2vF10qw4agF12yw4rJw1F vF4av3s8u3WYvrJanT9S1TB71UUUUUDqnTZGkaVYY2UrUUUUj1kv1TuYvTs0mT0YCTnIWj qI5I8CrVACY4xI64kE6c02F40Ex7xfYxn0WfASr-VFAUDa7-sFnT9fnUUIcSsGvfJTRUUU bxkYFVCjjxCrM7AC8VAFwI0_Jr0_Gr1l1xkIjI8I6I8E6xAIw20EY4v20xvaj40_Wr0E3s 1l1IIY67AEw4v_Jr0_Jr4l8cAvFVAK0II2c7xJM28CjxkF64kEwVA0rcxSw2x7M28EF7xv wVC0I7IYx2IY67AKxVWUCVW8JwA2z4x0Y4vE2Ix0cI8IcVCY1x0267AKxVWUJVW8JwA2z4 x0Y4vEx4A2jsIE14v26F4j6r4UJwA2z4x0Y4vEx4A2jsIEc7CjxVAFwI0_Gr0_Gr1UM2AI xVAIcxkEcVAq07x20xvEncxIr21l57IF6xkI12xvs2x26I8E6xACxx1l5I8CrVACY4xI64 kE6c02F40Ex7xfMcIj6xIIjxv20xvE14v26r1Y6r17McIj6I8E87Iv67AKxVWUJVW8JwAm 72CE4IkC6x0Yz7v_Jr0_Gr1lF7xvr2IY64vIr41lc7I2V7IY0VAS07AlzVAYIcxG8wCF04 k20xvY0x0EwIxGrwCFx2IqxVCFs4IE7xkEbVWUJVW8JwC20s026c02F40E14v26r1j6r18 MI8I3I0E7480Y4vE14v26r106r1rMI8E67AF67kF1VAFwI0_Jw0_GFylIxkGc2Ij64vIr4 1lIxAIcVC0I7IYx2IY67AKxVWUJVWUCwCI42IY6xIIjxv20xvEc7CjxVAFwI0_Jr0_Gr1l IxAIcVCF04k26cxKx2IYs7xG6r1j6r1xMIIF0xvEx4A2jsIE14v26r1j6r4UMIIF0xvEx4 A2jsIEc7CjxVAFwI0_Jr0_GrUvcSsGvfC2KfnxnUUI43ZEXa7IU1QVy3UUUUU== X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00,BODY_8BITS,GIT_PATCH_0,KAM_DMARC_STATUS,KAM_SHORT,NICE_REPLY_A,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 and fix atomic_exchange. > 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.\\t%0,%1\\n\\t" >> +        "and\\t%7,%0,%z3\\n\\t" >> +        "or%i5\\t%7,%7,%5\\n\\t" >> +        "sc.\\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. :-)