This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFC PATCH, i386]: PR 36793: x86-64 does not get __sync_synchronize right


On Sun, Nov 23, 2008 at 6:32 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> Hello!
>>
>> This patch just wires "memory_barrier" named pattern to mfence insn.
>>
>> 2008-11-22  Uros Bizjak  <ubizjak@gmail.com>
>>
>>   PR target/36793
>>   * config/i386/sync.md (memory_barrier): New expander.
>>
>> Patch was bootstrapped and regression tested on x86_64-pc-linux-gnu
>> {,-m32}.
>>
>> I will wait a day or two before for possible comments on this patch.
>>
>> BTW: "lock nop" as suggested in PR for !TARGET_SSE2 case generates invalid
>> insn exception.
>
>> It needs to be a "nop" that modifies memory.  Like "lock orb $0,(%esp)"
>> or "xchgl eax,dummy".
>
> Attached patch implements the suggestion with locked no-operation insn. On
> !TARGET_SSE2, we generate:
>
>   lock orb    $0, -1(%esp)
>
> where memory location gets assigned by assign_386_stack_local (). This way,
> we avoid various store forwarding stalls, since this is an exclusive QImode
> address. Also, it can point into redzone area, when redzone is available.
>
> (BTW: Patch also cleanups usage of UNSPECV_CMPXCHG*.)
>
> Patch was bootstrapped and regression tested on x86_64-pc-linux-gnu. I will
> wait a day or two for possible comments before committing it ti SVN.
>
> Thanks,
> Uros.
>
> Index: config/i386/i386.md
> ===================================================================
> --- config/i386/i386.md (revision 142131)
> +++ config/i386/i386.md (working copy)
> @@ -219,8 +219,7 @@
>    (UNSPECV_ALIGN              7)
>    (UNSPECV_MONITOR            8)
>    (UNSPECV_MWAIT              9)
> -   (UNSPECV_CMPXCHG_1          10)
> -   (UNSPECV_CMPXCHG_2          11)
> +   (UNSPECV_CMPXCHG            10)
>    (UNSPECV_XCHG               12)
>    (UNSPECV_LOCK               13)
>    (UNSPECV_PROLOGUE_USE       14)
> Index: config/i386/sync.md
> ===================================================================
> --- config/i386/sync.md (revision 142131)
> +++ config/i386/sync.md (working copy)
> @@ -31,6 +31,25 @@
>  (define_mode_attr doublemodesuffix [(DI "8") (TI "16")])
>  (define_mode_attr DCASHMODE [(DI "SI") (TI "DI")])
>
> +(define_expand "memory_barrier"
> +  [(set (match_dup 0)
> +       (unspec:BLK [(match_dup 0)] UNSPEC_MFENCE))]
> +  ""
> +{
> +  if (!TARGET_SSE2)
> +    {
> +      /* Emit a locked no-operation that accesses
> +        memory when SSE2 is not available.  */
> +      int slot = virtuals_instantiated ? SLOT_TEMP : SLOT_VIRTUAL;
> +      rtx temp = assign_386_stack_local (QImode, slot);
> +      emit_insn (gen_sync_iorqi (temp, CONST0_RTX (QImode)));
> +      DONE;
> +    }
> +
> +  operands[0] = gen_rtx_MEM (BLKmode, gen_rtx_SCRATCH (Pmode));
> +  MEM_VOLATILE_P (operands[0]) = 1;
> +})
> +
>  ;; ??? It would be possible to use cmpxchg8b on pentium for DImode
>  ;; changes.  It's complicated because the insn uses ecx:ebx as the
>  ;; new value; note that the registers are reversed from the order
> @@ -46,7 +65,7 @@
>            [(match_dup 1)
>             (match_operand:CASMODE 2 "register_operand" "")
>             (match_operand:CASMODE 3 "register_operand" "")]
> -           UNSPECV_CMPXCHG_1))
> +           UNSPECV_CMPXCHG))
>      (clobber (reg:CC FLAGS_REG))])]
>   "TARGET_CMPXCHG"
>  {
> @@ -78,10 +97,10 @@
>          [(match_dup 1)
>           (match_operand:IMODE 2 "register_operand" "a")
>           (match_operand:IMODE 3 "register_operand" "<modeconstraint>")]
> -         UNSPECV_CMPXCHG_1))
> +         UNSPECV_CMPXCHG))
>    (clobber (reg:CC FLAGS_REG))]
>   "TARGET_CMPXCHG"
> -  "lock{%;| } cmpxchg{<modesuffix>}\t{%3, %1|%1, %3}")
> +  "lock{%;| }cmpxchg{<modesuffix>}\t{%3, %1|%1, %3}")
>
>  (define_insn "sync_double_compare_and_swap<mode>"
>   [(set (match_operand:DCASMODE 0 "register_operand" "=A")
> @@ -92,7 +111,7 @@
>           (match_operand:DCASMODE 2 "register_operand" "A")
>           (match_operand:<DCASHMODE> 3 "register_operand" "b")
>           (match_operand:<DCASHMODE> 4 "register_operand" "c")]
> -         UNSPECV_CMPXCHG_1))
> +         UNSPECV_CMPXCHG))
>    (clobber (reg:CC FLAGS_REG))]
>   ""
>   "lock{%;| }cmpxchg<doublemodesuffix>b\t%1")
> @@ -115,7 +134,7 @@
>           (match_operand:DI 2 "register_operand" "A")
>           (match_operand:SI 3 "register_operand" "SD")
>           (match_operand:SI 4 "register_operand" "c")]
> -         UNSPECV_CMPXCHG_1))
> +         UNSPECV_CMPXCHG))
>    (clobber (reg:CC FLAGS_REG))]
>   "!TARGET_64BIT && TARGET_CMPXCHG8B && flag_pic"
>   "xchg{l}\t%%ebx, %3\;lock{%;| }cmpxchg8b\t%1\;xchg{l}\t%%ebx, %3")
> @@ -129,11 +148,11 @@
>            [(match_dup 1)
>             (match_operand:CASMODE 2 "register_operand" "")
>             (match_operand:CASMODE 3 "register_operand" "")]
> -           UNSPECV_CMPXCHG_1))
> +           UNSPECV_CMPXCHG))
>      (set (match_dup 4)
>          (compare:CCZ
>            (unspec_volatile:CASMODE
> -             [(match_dup 1) (match_dup 2) (match_dup 3)] UNSPECV_CMPXCHG_2)
> +             [(match_dup 1) (match_dup 2) (match_dup 3)] UNSPECV_CMPXCHG)
>            (match_dup 2)))])]
>   "TARGET_CMPXCHG"
>  {
> @@ -169,11 +188,11 @@
>          [(match_dup 1)
>           (match_operand:IMODE 2 "register_operand" "a")
>           (match_operand:IMODE 3 "register_operand" "<modeconstraint>")]
> -         UNSPECV_CMPXCHG_1))
> +         UNSPECV_CMPXCHG))
>    (set (reg:CCZ FLAGS_REG)
>        (compare:CCZ
>          (unspec_volatile:IMODE
> -           [(match_dup 1) (match_dup 2) (match_dup 3)] UNSPECV_CMPXCHG_2)
> +           [(match_dup 1) (match_dup 2) (match_dup 3)] UNSPECV_CMPXCHG)
>          (match_dup 2)))]
>   "TARGET_CMPXCHG"
>   "lock{%;| }cmpxchg{<modesuffix>}\t{%3, %1|%1, %3}")
> @@ -187,12 +206,12 @@
>           (match_operand:DCASMODE 2 "register_operand" "A")
>           (match_operand:<DCASHMODE> 3 "register_operand" "b")
>           (match_operand:<DCASHMODE> 4 "register_operand" "c")]
> -         UNSPECV_CMPXCHG_1))
> +         UNSPECV_CMPXCHG))
>    (set (reg:CCZ FLAGS_REG)
>        (compare:CCZ
>          (unspec_volatile:DCASMODE
>            [(match_dup 1) (match_dup 2) (match_dup 3) (match_dup 4)]
> -           UNSPECV_CMPXCHG_2)
> +           UNSPECV_CMPXCHG)
>          (match_dup 2)))]
>   ""
>   "lock{%;| }cmpxchg<doublemodesuffix>b\t%1")
> @@ -208,12 +227,12 @@
>           (match_operand:DI 2 "register_operand" "A")
>           (match_operand:SI 3 "register_operand" "SD")
>           (match_operand:SI 4 "register_operand" "c")]
> -         UNSPECV_CMPXCHG_1))
> +         UNSPECV_CMPXCHG))
>    (set (reg:CCZ FLAGS_REG)
>        (compare:CCZ
>          (unspec_volatile:DI
>            [(match_dup 1) (match_dup 2) (match_dup 3) (match_dup 4)]
> -           UNSPECV_CMPXCHG_2)
> +           UNSPECV_CMPXCHG)
>          (match_dup 2)))]
>   "!TARGET_64BIT && TARGET_CMPXCHG8B && flag_pic"
>   "xchg{l}\t%%ebx, %3\;lock{%;| }cmpxchg8b\t%1\;xchg{l}\t%%ebx, %3")
>
>

This patch caused

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38254

-- 
H.J.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]