Patchwork [off,list] Re: [PATCH] x86: use 'rep bsf' syntax when assembler supports it

login
register
mail settings
Submitter Roland McGrath
Date July 2, 2012, 6:39 p.m.
Message ID <CAB=4xhqe=orW8U7jiu7qxQOKjfd8x_jymNFffSHdAx1+bC6jug@mail.gmail.com>
Download mbox | patch
Permalink /patch/168628/
State New
Headers show

Comments

Roland McGrath - July 2, 2012, 6:39 p.m.
On Sun, Jul 1, 2012 at 1:08 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> Based on the observation above, the patch is OK for mainline, but
> please also handle "rep nop" case.

Here's the new version of the patch that does that.  Note that someone
needs to commit this for me, since I am not empowered to do it myself.


Thanks,
Roland


gcc/
2012-07-02  Roland McGrath  <mcgrathr@google.com>

	* configure.ac (HAVE_AS_IX86_REP_LOCK_PREFIX): Also require that the
	assembler accept 'rep bsf ...', 'rep bsr ...', 'rep ret', and 'rep nop'.
	* configure: Regenerated.
	* config/i386/i386.md (simple_return_internal_long): Use %;
	(ctz<mode>2): Likewise.
	(*pause): Likewise.
Uros Bizjak - July 3, 2012, 7:16 a.m.
On Mon, Jul 2, 2012 at 8:39 PM, Roland McGrath <mcgrathr@google.com> wrote:
> On Sun, Jul 1, 2012 at 1:08 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> Based on the observation above, the patch is OK for mainline, but
>> please also handle "rep nop" case.
>
> Here's the new version of the patch that does that.  Note that someone
> needs to commit this for me, since I am not empowered to do it myself.
>
> gcc/
> 2012-07-02  Roland McGrath  <mcgrathr@google.com>
>
>         * configure.ac (HAVE_AS_IX86_REP_LOCK_PREFIX): Also require that the
>         assembler accept 'rep bsf ...', 'rep bsr ...', 'rep ret', and 'rep nop'.
>         * configure: Regenerated.
>         * config/i386/i386.md (simple_return_internal_long): Use %;
>         (ctz<mode>2): Likewise.
>         (*pause): Likewise.

The patch is OK.

I have committed it to the mainline SVN.

Thanks,
Uros.
Richard Guenther - July 3, 2012, 10:37 a.m.
On Tue, Jul 3, 2012 at 9:16 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Mon, Jul 2, 2012 at 8:39 PM, Roland McGrath <mcgrathr@google.com> wrote:
>> On Sun, Jul 1, 2012 at 1:08 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>>> Based on the observation above, the patch is OK for mainline, but
>>> please also handle "rep nop" case.
>>
>> Here's the new version of the patch that does that.  Note that someone
>> needs to commit this for me, since I am not empowered to do it myself.
>>
>> gcc/
>> 2012-07-02  Roland McGrath  <mcgrathr@google.com>
>>
>>         * configure.ac (HAVE_AS_IX86_REP_LOCK_PREFIX): Also require that the
>>         assembler accept 'rep bsf ...', 'rep bsr ...', 'rep ret', and 'rep nop'.
>>         * configure: Regenerated.
>>         * config/i386/i386.md (simple_return_internal_long): Use %;
>>         (ctz<mode>2): Likewise.
>>         (*pause): Likewise.
>
> The patch is OK.
>
> I have committed it to the mainline SVN.

Given that the latest binutils release (2.22) does not support 'rep
ret', does that
not pessimize people with just "partial" 'rep *' support?

Richard.

> Thanks,
> Uros.
Uros Bizjak - July 3, 2012, 10:48 a.m.
On Tue, Jul 3, 2012 at 12:37 PM, Richard Guenther
<richard.guenther@gmail.com> wrote:

>>>> Based on the observation above, the patch is OK for mainline, but
>>>> please also handle "rep nop" case.
>>>
>>> Here's the new version of the patch that does that.  Note that someone
>>> needs to commit this for me, since I am not empowered to do it myself.
>>>
>>> gcc/
>>> 2012-07-02  Roland McGrath  <mcgrathr@google.com>
>>>
>>>         * configure.ac (HAVE_AS_IX86_REP_LOCK_PREFIX): Also require that the
>>>         assembler accept 'rep bsf ...', 'rep bsr ...', 'rep ret', and 'rep nop'.
>>>         * configure: Regenerated.
>>>         * config/i386/i386.md (simple_return_internal_long): Use %;
>>>         (ctz<mode>2): Likewise.
>>>         (*pause): Likewise.
>>
>> The patch is OK.
>>
>> I have committed it to the mainline SVN.
>
> Given that the latest binutils release (2.22) does not support 'rep
> ret', does that
> not pessimize people with just "partial" 'rep *' support?

No, gcc just emits "rep; ret". Using older binutils, this is just
cosmetic change in the asm dumps. IIUC, Roland worries that *future*
binutils could insert something after the "rep" in "rep;INSN" combo.

Uros.
Richard Guenther - July 3, 2012, 11:01 a.m.
On Tue, Jul 3, 2012 at 12:48 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Tue, Jul 3, 2012 at 12:37 PM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>
>>>>> Based on the observation above, the patch is OK for mainline, but
>>>>> please also handle "rep nop" case.
>>>>
>>>> Here's the new version of the patch that does that.  Note that someone
>>>> needs to commit this for me, since I am not empowered to do it myself.
>>>>
>>>> gcc/
>>>> 2012-07-02  Roland McGrath  <mcgrathr@google.com>
>>>>
>>>>         * configure.ac (HAVE_AS_IX86_REP_LOCK_PREFIX): Also require that the
>>>>         assembler accept 'rep bsf ...', 'rep bsr ...', 'rep ret', and 'rep nop'.
>>>>         * configure: Regenerated.
>>>>         * config/i386/i386.md (simple_return_internal_long): Use %;
>>>>         (ctz<mode>2): Likewise.
>>>>         (*pause): Likewise.
>>>
>>> The patch is OK.
>>>
>>> I have committed it to the mainline SVN.
>>
>> Given that the latest binutils release (2.22) does not support 'rep
>> ret', does that
>> not pessimize people with just "partial" 'rep *' support?
>
> No, gcc just emits "rep; ret". Using older binutils, this is just
> cosmetic change in the asm dumps. IIUC, Roland worries that *future*
> binutils could insert something after the "rep" in "rep;INSN" combo.

Ah, thanks for the clarification.  I've run into this because if you just
re-build in an already configured tree the cache variable is used and
the test is not re-done and you get unsupported rep ret ...

Richard.

> Uros.

Patch

diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index 60aa65a..f23c13c 100644
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -11904,7 +11904,7 @@ 
   [(simple_return)
    (unspec [(const_int 0)] UNSPEC_REP)]
   "reload_completed"
-  "rep\;ret"
+  "rep%; ret"
   [(set_attr "length" "2")
    (set_attr "atom_unit" "jeu")
    (set_attr "length_immediate" "0")
@@ -12236,8 +12236,8 @@ 
   else if (optimize_function_for_size_p (cfun))
     ;
   else if (TARGET_GENERIC)
-    /* tzcnt expands to rep;bsf and we can use it even if !TARGET_BMI.  */
-    return "rep; bsf{<imodesuffix>}\t{%1, %0|%0, %1}";
+    /* tzcnt expands to 'rep bsf' and we can use it even if !TARGET_BMI.  */
+    return "rep%; bsf{<imodesuffix>}\t{%1, %0|%0, %1}";

   return "bsf{<imodesuffix>}\t{%1, %0|%0, %1}";
 }
@@ -18131,7 +18131,7 @@ 
   [(set (match_operand:BLK 0)
 	(unspec:BLK [(match_dup 0)] UNSPEC_PAUSE))]
   ""
-  "rep; nop"
+  "rep%; nop"
   [(set_attr "length" "2")
    (set_attr "memory" "unknown")])

diff --git a/gcc/configure b/gcc/configure
index fd3be52..db81441 100755
--- a/gcc/configure
+++ b/gcc/configure
@@ -24791,7 +24791,11 @@  else
   if test x$gcc_cv_as != x; then
     $as_echo 'rep movsl
 	 lock addl %edi, (%eax,%esi)
-	 lock orl $0, (%esp)' > conftest.s
+	 lock orl $0, (%esp)
+	 rep bsf %ecx, %eax
+	 rep bsr %ecx, %eax
+	 rep ret
+	 rep nop' > conftest.s
     if { ac_try='$gcc_cv_as $gcc_cv_as_flags  -o conftest.o conftest.s >&5'
   { { eval echo "\"\$as_me\":${as_lineno-$LINENO}: \"$ac_try\""; } >&5
   (eval $ac_try) 2>&5
@@ -28693,4 +28697,3 @@  if test -n "$ac_unrecognized_opts" && test
"$enable_option_checking" != no; then
 $as_echo "$as_me: WARNING: unrecognized options: $ac_unrecognized_opts" >&2;}
 fi

-
diff --git a/gcc/configure.ac b/gcc/configure.ac
index 89644e2..3bc523d 100644
--- a/gcc/configure.ac
+++ b/gcc/configure.ac
@@ -3642,7 +3642,11 @@  foo:	nop
         gcc_cv_as_ix86_rep_lock_prefix,,,
 	[rep movsl
 	 lock addl %edi, (%eax,%esi)
-	 lock orl $0, (%esp)],,
+	 lock orl $0, (%esp)
+	 rep bsf %ecx, %eax
+	 rep bsr %ecx, %eax
+	 rep ret
+	 rep nop],,
         [AC_DEFINE(HAVE_AS_IX86_REP_LOCK_PREFIX, 1,
           [Define if the assembler supports 'rep <insn>, lock <insn>'.])])

@@ -5158,4 +5162,3 @@  done
 ],
 [subdirs='$subdirs'])
 AC_OUTPUT
-