From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <gcc-patches-return-335420-listarch-gcc-patches=gcc.gnu.org@gcc.gnu.org>
Received: (qmail 21841 invoked by alias); 13 Jan 2013 20:36:13 -0000
Received: (qmail 21664 invoked by uid 22791); 13 Jan 2013 20:36:12 -0000
X-SWARE-Spam-Status: No, hits=-2.0 required=5.0	tests=AWL,BAYES_00,RP_MATCHES_RCVD,TW_ZJ
X-Spam-Check-By: sourceware.org
Received: from one.firstfloor.org (HELO one.firstfloor.org) (213.235.205.2)    by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 13 Jan 2013 20:36:05 +0000
Received: by one.firstfloor.org (Postfix, from userid 503)	id 053D51ED8025; Sun, 13 Jan 2013 21:36:04 +0100 (CET)
Date: Sun, 13 Jan 2013 20:36:00 -0000
From: Andi Kleen <andi@firstfloor.org>
To: Uros Bizjak <ubizjak@gmail.com>
Cc: gcc-patches@gcc.gnu.org, Andi Kleen <andi@firstfloor.org>
Subject: Re: [PATCH 2/2] Support __ATOMIC_HLE_RELEASE for __atomic_clear/store_n
Message-ID: <20130113203603.GS30577@one.firstfloor.org>
References: <CAFULd4Ztbccsq9h50Tp-0JGHr4CoFJ2QByf0WgS4Ed92_09=Rw@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAFULd4Ztbccsq9h50Tp-0JGHr4CoFJ2QByf0WgS4Ed92_09=Rw@mail.gmail.com>
User-Agent: Mutt/1.4.2.2i
Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-patches.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-patches/>
List-Post: <mailto:gcc-patches@gcc.gnu.org>
List-Help: <mailto:gcc-patches-help@gcc.gnu.org>
Sender: gcc-patches-owner@gcc.gnu.org
X-SW-Source: 2013-01/txt/msg00663.txt.bz2

On Sun, Jan 13, 2013 at 08:59:15PM +0100, Uros Bizjak wrote:
> Hello!
> 
> > __atomic_clear and __atomic_store_n didn't have code to generate
> > the TSX HLE RELEASE prefix. Add this plus test cases.
> 
> +(define_insn "atomic_store_hle_release<mode>"
> +  [(set (match_operand:ATOMIC 0 "memory_operand")
> +	(unspec:ATOMIC [(match_operand:ATOMIC 1 "register_operand")
> +			(match_operand:SI 2 "const_int_operand")]
> +		       UNSPEC_MOVA_RELEASE))]
> +  ""
> +  "%K2mov{<imodesuffix>}\t{%1, %0|%0, %1}")
> 
> This pattern doesn't have any constraints! Also, mov insn can store
> immediates directly.

Can you suggest a better pattern?

> 
> +      if (model & IX86_HLE_RELEASE)
> +        {
> +      	  emit_insn (gen_atomic_store_hle_release<mode> (operands[0],
> operands[1],
> +	  	  				         operands[2]));
> +	  DONE;
> +        }					
> +
>        /* For seq-cst stores, when we lack MFENCE, use XCHG.  */
>        if (model == MEMMODEL_SEQ_CST && !(TARGET_64BIT || TARGET_SSE2))
> 
> What about __ATOMIC_SEQ_CST; should
> 
>   __atomic_clear (p, __ATOMIC_SEQ_CST | __ATOMIC_HLE_RELEASE);
> 
> emit a mfence at the end; in case of for your test:

Originally I thought not, but now on reconsideration it's needed for
older CPUs that don't know about the XRELEASE. And it may be even needed
with TSX for the non transactional fallback execution. I'll fix the patch.

> +
> +void
> +hle_clear (int *p, int v)
> 
> hle_clear (char *p)
> 
> This argument should correspond to a bool, please see documentation.


Not sure I understand? Which documentation? This is just a random test case

>    DONE;
>  })
>  
> +(define_insn "atomic_store<mode>_1"
> +  [(set (match_operand:ATOMIC 0 "memory_operand" "=m")
> +	(unspec:ATOMIC [(match_operand:ATOMIC 1 "<nonmemory_operand>" "<r><i>")
> +			(match_operand:SI 2 "const_int_operand")]
> +		       UNSPEC_MOVA))]
> +  ""
> +  "%K2mov{<imodesuffix>}\t{%1, %0|%0, %1}")

Is that the updated pattern you wanted? It looks similar to mine.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.