From patchwork Sun May 29 03:17:52 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Oleg Endo X-Patchwork-Id: 627473 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3rHQ284cxnz9t3l for ; Sun, 29 May 2016 13:18:39 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=wAXezt7U; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:subject:from:to:date:in-reply-to:references :content-type:mime-version; q=dns; s=default; b=VUILEryGm7jl4/Xx njWcjYcsOE5DElg3EDP17x/NeqgfND5k5YPRPo3aCyDr6HCcu7ZRS76fjGgqmvMB 4YFP7m9Jjy325BiYEceBXm4dLu2JX1XQnPT+pE+PxPpLH1dbn7USelCqRi0Ci9LL ZNEv0/WiLGxlAzinUR54V4ogSZA= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:subject:from:to:date:in-reply-to:references :content-type:mime-version; s=default; bh=I1TiS1+23AJafQrSxg46Y3 RhRrM=; b=wAXezt7Uzfa2BGSXr6NEjeeIUVgJm4HPko1C3Ov02v4VIO0DKKga7W Zlm8L0ak8xKrAmovERETqzOrzRgj8FwIM7ZEZQQXFna/vN39z8IXb0I7iHqESruL /ZJuJgHM+Uba7zyBAJ9ZhXmQhvYUf6Em1VQtqb9j0HWQV28/rvBVg= Received: (qmail 41984 invoked by alias); 29 May 2016 03:18:27 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 33685 invoked by uid 89); 29 May 2016 03:18:18 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL, BAYES_00, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_NONE, RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=ri, restoring, twist, patterns X-HELO: mailout05.t-online.de Received: from mailout05.t-online.de (HELO mailout05.t-online.de) (194.25.134.82) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Sun, 29 May 2016 03:18:07 +0000 Received: from fwd02.aul.t-online.de (fwd02.aul.t-online.de [172.20.26.148]) by mailout05.t-online.de (Postfix) with SMTP id 29CF64230EA2; Sun, 29 May 2016 05:18:01 +0200 (CEST) Received: from [192.168.0.16] (TW0GbcZ6gh1oQsQgFK+deJx3LMOd7kFkFo1m-4u0wnALPMxfrIw+znB2CHdTn13wwO@[115.165.93.200]) by fwd02.t-online.de with (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384 encrypted) esmtp id 1b6rEh-21l7lg0; Sun, 29 May 2016 05:17:55 +0200 Message-ID: <1464491872.2269.93.camel@t-online.de> Subject: Re: [RX] Add support for atomic operations From: Oleg Endo To: Nick Clifton , gcc-patches Date: Sun, 29 May 2016 12:17:52 +0900 In-Reply-To: <97fe59ae-2dc6-4684-0fd6-f976a1c97870@redhat.com> References: <1462703977.3864.32.camel@t-online.de> <97fe59ae-2dc6-4684-0fd6-f976a1c97870@redhat.com> Mime-Version: 1.0 X-IsSubscribed: yes On Mon, 2016-05-09 at 15:18 +0100, Nick Clifton wrote: > > gcc/ChangeLog: > > * config/rx/rx-protos.h (is_interrupt_func, > > is_fast_interrupt_func): > > Forward declare. > > (rx_atomic_sequence): New class. > > * config/rx/rx.c (rx_print_operand): Use symbolic names for PSW > > bits. > > (is_interrupt_func, is_fast_interrupt_func): Make non-static > > and > > non-inline. > > (rx_atomic_sequence::rx_atomic_sequence, > > rx_atomic_sequence::~rx_atomic_sequence): New functions. > > * config/rx/rx.md (CTRLREG_PSW, CTRLREG_USP, CTRLREG_FPSW, > > CTRLREG_CPEN, > > CTRLREG_BPSW, CTRLREG_BPC, CTRLREG_ISP, CTRLREG_FINTV, > > CTRLREG_INTB): New constants. > > (FETCHOP): New code iterator. > > (fethcop_name, fetchop_name2): New iterator code attributes. > > (QIHI): New mode iterator. > > (atomic_exchange, atomic_exchangesi, xchg_mem, > > atomic_fetch_si, atomic_fetch_nandsi, > > atomic__fetchsi, atomic_nand_fetchsi): New > > patterns. > > Approved - please apply. > Sorry, but my original patch was buggy. There are two problems: First, when interrupts are re-enabled by restoring the PSW using the "mvtc" insn after the atomic sequence, the CC_REG is clobbered. It's not entirely clear to me why leaving out the CC_REG clobber in "mvtc" is of any benefit. Instead of adding a new "mvtc" pattern, I've just added the clobber to the existing one. With that wrong code issues around atomic sequences such as atomic decrement and test for zero are fixed. Second, the atomic__fetchsi works only with commutative operations because the memory operand and the register operand are swapped in the expander. Thus it produces wrong code for subtraction operations. The fix is to use a separate pattern for subtraction and not twist the operands. The attached patch fixes those issues. OK for trunk? Cheers, Oleg gcc/ChangeLog: * config/rx/rx.md (FETCHOP_NO_MINUS): New code iterator. (atomic__fetchsi): Extract minus operator into ... (atomic_sub_fetchsi): ... this new pattern. (mvtc): Add CC_REG clobber. Index: gcc/config/rx/rx.md =================================================================== --- gcc/config/rx/rx.md (revision 236761) +++ gcc/config/rx/rx.md (working copy) @@ -2158,6 +2158,7 @@ ;; Atomic operations. (define_code_iterator FETCHOP [plus minus ior xor and]) +(define_code_iterator FETCHOP_NO_MINUS [plus ior xor and]) (define_code_attr fetchop_name [(plus "add") (minus "sub") (ior "or") (xor "xor") (and "and")]) @@ -2258,12 +2259,14 @@ }) ;; read - modify - write - return new value +;; Because subtraction is not commutative we need to specify a different +;; set of patterns for it. (define_expand "atomic__fetchsi" [(set (match_operand:SI 0 "register_operand") - (FETCHOP:SI (match_operand:SI 1 "rx_restricted_mem_operand") - (match_operand:SI 2 "register_operand"))) + (FETCHOP_NO_MINUS:SI (match_operand:SI 1 "rx_restricted_mem_operand") + (match_operand:SI 2 "register_operand"))) (set (match_dup 1) - (FETCHOP:SI (match_dup 1) (match_dup 2))) + (FETCHOP_NO_MINUS:SI (match_dup 1) (match_dup 2))) (match_operand:SI 3 "const_int_operand")] ;; memory model "" { @@ -2277,6 +2280,25 @@ DONE; }) +(define_expand "atomic_sub_fetchsi" + [(set (match_operand:SI 0 "register_operand") + (minus:SI (match_operand:SI 1 "rx_restricted_mem_operand") + (match_operand:SI 2 "rx_source_operand"))) + (set (match_dup 1) + (minus:SI (match_dup 1) (match_dup 2))) + (match_operand:SI 3 "const_int_operand")] ;; memory model + "" +{ + { + rx_atomic_sequence seq (current_function_decl); + + emit_move_insn (operands[0], operands[1]); + emit_insn (gen_subsi3 (operands[0], operands[0], operands[2])); + emit_move_insn (operands[1], operands[0]); + } + DONE; +}) + (define_expand "atomic_nand_fetchsi" [(set (match_operand:SI 0 "register_operand") (not:SI (and:SI (match_operand:SI 1 "rx_restricted_mem_operand") @@ -2674,18 +2696,16 @@ ) ;; Move to control register +;; This insn can be used in atomic sequences to restore the previous PSW +;; and re-enable interrupts. Because of that it always clobbers the CC_REG. (define_insn "mvtc" [(unspec_volatile:SI [(match_operand:SI 0 "immediate_operand" "i,i") (match_operand:SI 1 "nonmemory_operand" "r,i")] - UNSPEC_BUILTIN_MVTC)] + UNSPEC_BUILTIN_MVTC) + (clobber (reg:CC CC_REG))] "" "mvtc\t%1, %C0" [(set_attr "length" "3,7")] - ;; Ignore possible clobbering of the comparison flags in the - ;; PSW register. This is a cc0 target so any cc0 setting - ;; instruction will always be paired with a cc0 user, without - ;; the possibility of this instruction being placed in between - ;; them. ) ;; Move to interrupt priority level