[PATCH] Add memory barriers to xbegin/xend/xabort

[PATCH] Add memory barriers to xbegin/xend/xabort

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

xbegin/xend/xabort were missing memory barriers. This can
lead to memory operations being moved out of transactions, which would
cause unexpected races.

Always generate implicit memory barriers for these intrinsics.

The compat header versions always generated memory barriers,
so this also improves compatibility.

Passes test suite. Ok for release branches?

gcc/:

2014-10-28  Andi Kleen  <ak@linux.intel.com>

	PR target/63672
	* config/i386/i386.c (ix86_expand_builtin): Generate memory
	barrier after abort.
	* config/i386/i386.md (xbegin): Add memory barrier.
	(xend): Rename to ...
	(xend_1): New. Generate memory barrier and emit xend.
---
 gcc/config/i386/i386.c  |  1 +
 gcc/config/i386/i386.md | 18 +++++++++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index ec3e056..ec0df40 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -36413,6 +36413,7 @@ addcarryx:
 	  return const0_rtx;
 	}
       emit_insn (gen_xabort (op0));
+      emit_insn (gen_memory_blockage ());
       return 0;
 
     default:
diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index 7ba07c3..3544e60 100644
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -18530,6 +18530,9 @@
 
   emit_move_insn (operands[0], ax_reg);
 
+  operands[0] = gen_rtx_MEM (BLKmode, gen_rtx_SCRATCH (Pmode));
+  MEM_VOLATILE_P (operands[0]) = 1;
+
   DONE;
 })
 
@@ -18546,13 +18549,26 @@
   [(set_attr "type" "other")
    (set_attr "length" "6")])
 
-(define_insn "xend"
+(define_insn "xend_1"
   [(unspec_volatile [(const_int 0)] UNSPECV_XEND)]
   "TARGET_RTM"
   "xend"
   [(set_attr "type" "other")
    (set_attr "length" "3")])
 
+(define_expand "xend"
+  [(set (match_dup 0)
+	(unspec:BLK [(const_int 0)] UNSPECV_XEND))] /* or match_dup 0 ? */
+  "TARGET_RTM"
+{
+  emit_insn (gen_xend_1 ());
+
+  operands[0] = gen_rtx_MEM (BLKmode, gen_rtx_SCRATCH (Pmode));
+  MEM_VOLATILE_P (operands[0]) = 1;
+
+  DONE;
+})
+
 (define_insn "xabort"
   [(unspec_volatile [(match_operand:SI 0 "const_0_to_255_operand" "n")]
 		    UNSPECV_XABORT)]
-- 
2.1.1

Re: [PATCH] Add memory barriers to xbegin/xend/xabort

[Richard Biener]

On Wed, Oct 29, 2014 at 4:31 AM, Andi Kleen <andi@firstfloor.org> wrote:
> From: Andi Kleen <ak@linux.intel.com>
>
> xbegin/xend/xabort were missing memory barriers. This can
> lead to memory operations being moved out of transactions, which would
> cause unexpected races.
>
> Always generate implicit memory barriers for these intrinsics.
>
> The compat header versions always generated memory barriers,
> so this also improves compatibility.
>
> Passes test suite. Ok for release branches?

Hmm, can't the insns themselves properly clobber/use memory?
I suppose they are UNSPEC_VOLATILE anyway, right?

Richard.

> gcc/:
>
> 2014-10-28  Andi Kleen  <ak@linux.intel.com>
>
>         PR target/63672
>         * config/i386/i386.c (ix86_expand_builtin): Generate memory
>         barrier after abort.
>         * config/i386/i386.md (xbegin): Add memory barrier.
>         (xend): Rename to ...
>         (xend_1): New. Generate memory barrier and emit xend.
> ---
>  gcc/config/i386/i386.c  |  1 +
>  gcc/config/i386/i386.md | 18 +++++++++++++++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index ec3e056..ec0df40 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -36413,6 +36413,7 @@ addcarryx:
>           return const0_rtx;
>         }
>        emit_insn (gen_xabort (op0));
> +      emit_insn (gen_memory_blockage ());
>        return 0;
>
>      default:
> diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
> index 7ba07c3..3544e60 100644
> --- a/gcc/config/i386/i386.md
> +++ b/gcc/config/i386/i386.md
> @@ -18530,6 +18530,9 @@
>
>    emit_move_insn (operands[0], ax_reg);
>
> +  operands[0] = gen_rtx_MEM (BLKmode, gen_rtx_SCRATCH (Pmode));
> +  MEM_VOLATILE_P (operands[0]) = 1;
> +
>    DONE;
>  })
>
> @@ -18546,13 +18549,26 @@
>    [(set_attr "type" "other")
>     (set_attr "length" "6")])
>
> -(define_insn "xend"
> +(define_insn "xend_1"
>    [(unspec_volatile [(const_int 0)] UNSPECV_XEND)]
>    "TARGET_RTM"
>    "xend"
>    [(set_attr "type" "other")
>     (set_attr "length" "3")])
>
> +(define_expand "xend"
> +  [(set (match_dup 0)
> +       (unspec:BLK [(const_int 0)] UNSPECV_XEND))] /* or match_dup 0 ? */
> +  "TARGET_RTM"
> +{
> +  emit_insn (gen_xend_1 ());
> +
> +  operands[0] = gen_rtx_MEM (BLKmode, gen_rtx_SCRATCH (Pmode));
> +  MEM_VOLATILE_P (operands[0]) = 1;
> +
> +  DONE;
> +})
> +
>  (define_insn "xabort"
>    [(unspec_volatile [(match_operand:SI 0 "const_0_to_255_operand" "n")]
>                     UNSPECV_XABORT)]
> --
> 2.1.1
>

Re: [PATCH] Add memory barriers to xbegin/xend/xabort

[Andi Kleen]

> 
> Hmm, can't the insns themselves properly clobber/use memory?

The transactions don't really use the memory. They just guard it,
like a lock.

So the intrinsic doesn't know what memory is used inside the transaction,
but the accesses still cannot be moved out.

I think a barrier is the only sensible option.

> I suppose they are UNSPEC_VOLATILE anyway, right?

That doesn't have any barrier semantics by itself, does it?

-Andi

[PING] Re: [PATCH] Add memory barriers to xbegin/xend/xabort

[Andi Kleen]

Andi Kleen <andi@firstfloor.org> writes:

Ping!

> From: Andi Kleen <ak@linux.intel.com>
>
> xbegin/xend/xabort were missing memory barriers. This can
> lead to memory operations being moved out of transactions, which would
> cause unexpected races.
>
> Always generate implicit memory barriers for these intrinsics.
>
> The compat header versions always generated memory barriers,
> so this also improves compatibility.
>
> Passes test suite. Ok for release branches?
>
> gcc/:
>
> 2014-10-28  Andi Kleen  <ak@linux.intel.com>
>
> 	PR target/63672
> 	* config/i386/i386.c (ix86_expand_builtin): Generate memory
> 	barrier after abort.
> 	* config/i386/i386.md (xbegin): Add memory barrier.
> 	(xend): Rename to ...
> 	(xend_1): New. Generate memory barrier and emit xend.
> ---
>  gcc/config/i386/i386.c  |  1 +
>  gcc/config/i386/i386.md | 18 +++++++++++++++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index ec3e056..ec0df40 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -36413,6 +36413,7 @@ addcarryx:
>  	  return const0_rtx;
>  	}
>        emit_insn (gen_xabort (op0));
> +      emit_insn (gen_memory_blockage ());
>        return 0;
>  
>      default:
> diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
> index 7ba07c3..3544e60 100644
> --- a/gcc/config/i386/i386.md
> +++ b/gcc/config/i386/i386.md
> @@ -18530,6 +18530,9 @@
>  
>    emit_move_insn (operands[0], ax_reg);
>  
> +  operands[0] = gen_rtx_MEM (BLKmode, gen_rtx_SCRATCH (Pmode));
> +  MEM_VOLATILE_P (operands[0]) = 1;
> +
>    DONE;
>  })
>  
> @@ -18546,13 +18549,26 @@
>    [(set_attr "type" "other")
>     (set_attr "length" "6")])
>  
> -(define_insn "xend"
> +(define_insn "xend_1"
>    [(unspec_volatile [(const_int 0)] UNSPECV_XEND)]
>    "TARGET_RTM"
>    "xend"
>    [(set_attr "type" "other")
>     (set_attr "length" "3")])
>  
> +(define_expand "xend"
> +  [(set (match_dup 0)
> +	(unspec:BLK [(const_int 0)] UNSPECV_XEND))] /* or match_dup 0 ? */
> +  "TARGET_RTM"
> +{
> +  emit_insn (gen_xend_1 ());
> +
> +  operands[0] = gen_rtx_MEM (BLKmode, gen_rtx_SCRATCH (Pmode));
> +  MEM_VOLATILE_P (operands[0]) = 1;
> +
> +  DONE;
> +})
> +
>  (define_insn "xabort"
>    [(unspec_volatile [(match_operand:SI 0 "const_0_to_255_operand" "n")]
>  		    UNSPECV_XABORT)]

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

Re: [PING^2] Re: [PATCH] Add memory barriers to xbegin/xend/xabort

[Andi Kleen]

Andi Kleen <andi@firstfloor.org> writes:

Ping^2!

> Andi Kleen <andi@firstfloor.org> writes:
>
> Ping!
>
>> From: Andi Kleen <ak@linux.intel.com>
>>
>> xbegin/xend/xabort were missing memory barriers. This can
>> lead to memory operations being moved out of transactions, which would
>> cause unexpected races.
>>
>> Always generate implicit memory barriers for these intrinsics.
>>
>> The compat header versions always generated memory barriers,
>> so this also improves compatibility.
>>
>> Passes test suite. Ok for release branches?
>>
>> gcc/:
>>
>> 2014-10-28  Andi Kleen  <ak@linux.intel.com>
>>
>> 	PR target/63672
>> 	* config/i386/i386.c (ix86_expand_builtin): Generate memory
>> 	barrier after abort.
>> 	* config/i386/i386.md (xbegin): Add memory barrier.
>> 	(xend): Rename to ...
>> 	(xend_1): New. Generate memory barrier and emit xend.
>> ---
>>  gcc/config/i386/i386.c  |  1 +
>>  gcc/config/i386/i386.md | 18 +++++++++++++++++-
>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
>> index ec3e056..ec0df40 100644
>> --- a/gcc/config/i386/i386.c
>> +++ b/gcc/config/i386/i386.c
>> @@ -36413,6 +36413,7 @@ addcarryx:
>>  	  return const0_rtx;
>>  	}
>>        emit_insn (gen_xabort (op0));
>> +      emit_insn (gen_memory_blockage ());
>>        return 0;
>>  
>>      default:
>> diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
>> index 7ba07c3..3544e60 100644
>> --- a/gcc/config/i386/i386.md
>> +++ b/gcc/config/i386/i386.md
>> @@ -18530,6 +18530,9 @@
>>  
>>    emit_move_insn (operands[0], ax_reg);
>>  
>> +  operands[0] = gen_rtx_MEM (BLKmode, gen_rtx_SCRATCH (Pmode));
>> +  MEM_VOLATILE_P (operands[0]) = 1;
>> +
>>    DONE;
>>  })
>>  
>> @@ -18546,13 +18549,26 @@
>>    [(set_attr "type" "other")
>>     (set_attr "length" "6")])
>>  
>> -(define_insn "xend"
>> +(define_insn "xend_1"
>>    [(unspec_volatile [(const_int 0)] UNSPECV_XEND)]
>>    "TARGET_RTM"
>>    "xend"
>>    [(set_attr "type" "other")
>>     (set_attr "length" "3")])
>>  
>> +(define_expand "xend"
>> +  [(set (match_dup 0)
>> +	(unspec:BLK [(const_int 0)] UNSPECV_XEND))] /* or match_dup 0 ? */
>> +  "TARGET_RTM"
>> +{
>> +  emit_insn (gen_xend_1 ());
>> +
>> +  operands[0] = gen_rtx_MEM (BLKmode, gen_rtx_SCRATCH (Pmode));
>> +  MEM_VOLATILE_P (operands[0]) = 1;
>> +
>> +  DONE;
>> +})
>> +
>>  (define_insn "xabort"
>>    [(unspec_volatile [(match_operand:SI 0 "const_0_to_255_operand" "n")]
>>  		    UNSPECV_XABORT)]

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

Re: [PATCH] Add memory barriers to xbegin/xend/xabort

[H.J. Lu]

On Wed, Oct 29, 2014 at 11:07 PM, Andi Kleen <ak@linux.intel.com> wrote:
>>
>> Hmm, can't the insns themselves properly clobber/use memory?
>
> The transactions don't really use the memory. They just guard it,
> like a lock.
>
> So the intrinsic doesn't know what memory is used inside the transaction,
> but the accesses still cannot be moved out.
>
> I think a barrier is the only sensible option.
>
>> I suppose they are UNSPEC_VOLATILE anyway, right?
>
> That doesn't have any barrier semantics by itself, does it?
>

Do you have a testcase to show it makes a difference?

-- 
H.J.

Re: [PATCH] Add memory barriers to xbegin/xend/xabort

[Andi Kleen]

"H.J. Lu" <hjl.tools@gmail.com> writes:

> On Wed, Oct 29, 2014 at 11:07 PM, Andi Kleen <ak@linux.intel.com> wrote:
>>>
>>> Hmm, can't the insns themselves properly clobber/use memory?
>>
>> The transactions don't really use the memory. They just guard it,
>> like a lock.
>>
>> So the intrinsic doesn't know what memory is used inside the transaction,
>> but the accesses still cannot be moved out.
>>
>> I think a barrier is the only sensible option.
>>
>>> I suppose they are UNSPEC_VOLATILE anyway, right?
>>
>> That doesn't have any barrier semantics by itself, does it?
>>
>
> Do you have a testcase to show it makes a difference?

It fixed customer code. I currently don't have a separate
test case.

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

Re: [PATCH] Add memory barriers to xbegin/xend/xabort

[H.J. Lu]

On Mon, Nov 17, 2014 at 12:36 PM, Andi Kleen <andi@firstfloor.org> wrote:
> "H.J. Lu" <hjl.tools@gmail.com> writes:
>
>> On Wed, Oct 29, 2014 at 11:07 PM, Andi Kleen <ak@linux.intel.com> wrote:
>>>>
>>>> Hmm, can't the insns themselves properly clobber/use memory?
>>>
>>> The transactions don't really use the memory. They just guard it,
>>> like a lock.
>>>
>>> So the intrinsic doesn't know what memory is used inside the transaction,
>>> but the accesses still cannot be moved out.
>>>
>>> I think a barrier is the only sensible option.
>>>
>>>> I suppose they are UNSPEC_VOLATILE anyway, right?
>>>
>>> That doesn't have any barrier semantics by itself, does it?
>>>
>>
>> Do you have a testcase to show it makes a difference?
>
> It fixed customer code. I currently don't have a separate
> test case.
>

If you can extract a small testcase, it will be much easier to review.

-- 
H.J.

Re: [PATCH] Add memory barriers to xbegin/xend/xabort

[Richard Henderson]

On 10/29/2014 04:31 AM, Andi Kleen wrote:
> 2014-10-28  Andi Kleen  <ak@linux.intel.com>
> 
> 	PR target/63672
> 	* config/i386/i386.c (ix86_expand_builtin): Generate memory
> 	barrier after abort.
> 	* config/i386/i386.md (xbegin): Add memory barrier.
> 	(xend): Rename to ...
> 	(xend_1): New. Generate memory barrier and emit xend.

Richi's comment is spot on.

The insns themselves should hold the barrier, not being separate like

> 
> -(define_insn "xend"
> +(define_insn "xend_1"
>    [(unspec_volatile [(const_int 0)] UNSPECV_XEND)]
>    "TARGET_RTM"
>    "xend"
>    [(set_attr "type" "other")
>     (set_attr "length" "3")])
>  
> +(define_expand "xend"
> +  [(set (match_dup 0)
> +	(unspec:BLK [(const_int 0)] UNSPECV_XEND))] /* or match_dup 0 ? */
> +  "TARGET_RTM"
> +{
> +  emit_insn (gen_xend_1 ());
> +
> +  operands[0] = gen_rtx_MEM (BLKmode, gen_rtx_SCRATCH (Pmode));
> +  MEM_VOLATILE_P (operands[0]) = 1;
> +
> +  DONE;
> +})

this, which generates two separate insns.  C.f. sse2_lfence.


r~