| Message ID | CAB=4xhqe=orW8U7jiu7qxQOKjfd8x_jymNFffSHdAx1+bC6jug@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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.
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.
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 -