Re: [PATCH 2/2] Support __ATOMIC_HLE_RELEASE for __atomic_clear/store_n

[Andi Kleen <andi@firstfloor.org>]

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.