Patchwork Vzeroupper placement/47440

login
register
mail settings
Submitter Jakub Jelinek
Date Nov. 6, 2012, 10:18 p.m.
Message ID <20121106221813.GZ1881@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/197562/
State New
Headers show

Comments

Jakub Jelinek - Nov. 6, 2012, 10:18 p.m.
On Tue, Nov 06, 2012 at 02:11:50PM -0800, H.J. Lu wrote:
> On Tue, Nov 6, 2012 at 2:30 AM, Kirill Yukhin <kirill.yukhin@gmail.com> wrote:
> > Hello,
> >> OK for mainline SVN, please commit.
> > Checked into GCC trunk: http://gcc.gnu.org/ml/gcc-cvs/2012-11/msg00176.html
> >
> > Thanks, K
> 
> This caused:
> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55224

Not only that, it also broke --enable-checking=yes,rtl bootstrap.
SET_DEST isn't valid on CALL, but XEXP (call, 0) is a MEM anyway and
the code looks for reg, so I think looking for CALL was just a mistake.

This fixes the bootstrap, ok for trunk?

2012-11-06  Jakub Jelinek  <jakub@redhat.com>

	* config/i386/i386.c (ix86_avx_u128_mode_after): Don't
	look for reg in CALL operand.



	Jakub
Uros Bizjak - Nov. 7, 2012, 7:03 a.m.
On Tue, Nov 6, 2012 at 11:18 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Nov 06, 2012 at 02:11:50PM -0800, H.J. Lu wrote:
>> On Tue, Nov 6, 2012 at 2:30 AM, Kirill Yukhin <kirill.yukhin@gmail.com> wrote:
>> > Hello,
>> >> OK for mainline SVN, please commit.
>> > Checked into GCC trunk: http://gcc.gnu.org/ml/gcc-cvs/2012-11/msg00176.html
>> >
>> > Thanks, K
>>
>> This caused:
>>
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55224
>
> Not only that, it also broke --enable-checking=yes,rtl bootstrap.
> SET_DEST isn't valid on CALL, but XEXP (call, 0) is a MEM anyway and
> the code looks for reg, so I think looking for CALL was just a mistake.
>
> This fixes the bootstrap, ok for trunk?
>
> 2012-11-06  Jakub Jelinek  <jakub@redhat.com>
>
>         * config/i386/i386.c (ix86_avx_u128_mode_after): Don't
>         look for reg in CALL operand.

OK.

Thanks,
Uros.
Uros Bizjak - Nov. 7, 2012, 7:08 a.m.
On Wed, Nov 7, 2012 at 8:03 AM, Uros Bizjak <ubizjak@gmail.com> wrote:

>>> >> OK for mainline SVN, please commit.
>>> > Checked into GCC trunk: http://gcc.gnu.org/ml/gcc-cvs/2012-11/msg00176.html
>>> >
>>> > Thanks, K
>>>
>>> This caused:
>>>
>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55224
>>
>> Not only that, it also broke --enable-checking=yes,rtl bootstrap.
>> SET_DEST isn't valid on CALL, but XEXP (call, 0) is a MEM anyway and
>> the code looks for reg, so I think looking for CALL was just a mistake.
>>
>> This fixes the bootstrap, ok for trunk?
>>
>> 2012-11-06  Jakub Jelinek  <jakub@redhat.com>
>>
>>         * config/i386/i386.c (ix86_avx_u128_mode_after): Don't
>>         look for reg in CALL operand.
>
> OK.

You can also break the loop after reg is found.

Uros.
Jakub Jelinek - Nov. 7, 2012, 8:04 a.m.
On Wed, Nov 07, 2012 at 08:08:08AM +0100, Uros Bizjak wrote:
> >> 2012-11-06  Jakub Jelinek  <jakub@redhat.com>
> >>
> >>         * config/i386/i386.c (ix86_avx_u128_mode_after): Don't
> >>         look for reg in CALL operand.
> >
> > OK.
> 
> You can also break the loop after reg is found.

I have committed the patch as is to fix the bootstrap, as anything else
needs another bootstrap/regtest.  I don't think breaking out of the loop
would be correct, then say for *{,sib}call_value_pop patterns reg would be
stack pointer rather than the return value of the call.  Due to that pattern
we can't use single_set, but I wonder if we just can't use XVECEXP (pat, 0, 0)
unconditionally for the return value, or perhaps check
the condition inside of the loop (REG_P (reg) && VALID_AVX256_REG_OR_OI_MODE (GET_MODE
(reg))), return AVX_U128_DIRTY if true (and that way break out of the loop),
and return AVX_U128_CLEAN after the loop.
Or I wonder why is call handled specially at all, doesn't
  /* Check if a 256bit AVX register is referenced in stores.  */
  state = unused;
  note_stores (pat, check_avx256_stores, &state);
  if (state == used)
    return AVX_U128_DIRTY;
handle it?  Then it would just need to be if (CALL_P (insn)) return AVX_U128_CLEAN.
BTW, the formatting is wrong in some spots, e.g.
check_avx256_stores (rtx dest, const_rtx set, void *data)
{
  if (((REG_P (dest) || MEM_P(dest))

I'd prefer to leave this to the original submitter.

	Jakub
Vladimir Yakovlev - Nov. 7, 2012, 3:09 p.m.
Hello,

Thanyou for investigation and fixing the problem.  I'll answer on remarks later.

Regards,
Vladimir

2012/11/7 Jakub Jelinek <jakub@redhat.com>:
> On Tue, Nov 06, 2012 at 02:11:50PM -0800, H.J. Lu wrote:
>> On Tue, Nov 6, 2012 at 2:30 AM, Kirill Yukhin <kirill.yukhin@gmail.com> wrote:
>> > Hello,
>> >> OK for mainline SVN, please commit.
>> > Checked into GCC trunk: http://gcc.gnu.org/ml/gcc-cvs/2012-11/msg00176.html
>> >
>> > Thanks, K
>>
>> This caused:
>>
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55224
>
> Not only that, it also broke --enable-checking=yes,rtl bootstrap.
> SET_DEST isn't valid on CALL, but XEXP (call, 0) is a MEM anyway and
> the code looks for reg, so I think looking for CALL was just a mistake.
>
> This fixes the bootstrap, ok for trunk?
>
> 2012-11-06  Jakub Jelinek  <jakub@redhat.com>
>
>         * config/i386/i386.c (ix86_avx_u128_mode_after): Don't
>         look for reg in CALL operand.
>
> --- gcc/config/i386/i386.c.jj   2012-11-06 18:10:22.000000000 +0100
> +++ gcc/config/i386/i386.c      2012-11-06 20:15:09.068912242 +0100
> @@ -15084,9 +15084,9 @@ ix86_avx_u128_mode_after (int mode, rtx
>    /* Check for CALL instruction.  */
>    if (CALL_P (insn))
>      {
> -      if (GET_CODE (pat) == SET || GET_CODE (pat) == CALL)
> +      if (GET_CODE (pat) == SET)
>         reg = SET_DEST (pat);
> -      else if (GET_CODE (pat) ==  PARALLEL)
> +      else if (GET_CODE (pat) == PARALLEL)
>         for (i = XVECLEN (pat, 0) - 1; i >= 0; i--)
>           {
>             rtx x = XVECEXP (pat, 0, i);
>
>
>         Jakub
Vladimir Yakovlev - Nov. 8, 2012, 5:49 a.m.
I tested changes with configure

../gcc/configure --enable-clocale=gnu --with-system-zlib
--enable-shared --with-demangler-in-ld --with-fpmath=sse
--enable-languages=c,c++,fortran,java,lto,objc --with-arch=corei7-avx
--with-cpu=corei7-avx

Bootstrap is passed and no new fails in make check.

Thank you,
Vladimir


2012/11/7 Vladimir Yakovlev <vbyakovl23@gmail.com>:
> Hello,
>
> Thanyou for investigation and fixing the problem.  I'll answer on remarks later.
>
> Regards,
> Vladimir
>
> 2012/11/7 Jakub Jelinek <jakub@redhat.com>:
>> On Tue, Nov 06, 2012 at 02:11:50PM -0800, H.J. Lu wrote:
>>> On Tue, Nov 6, 2012 at 2:30 AM, Kirill Yukhin <kirill.yukhin@gmail.com> wrote:
>>> > Hello,
>>> >> OK for mainline SVN, please commit.
>>> > Checked into GCC trunk: http://gcc.gnu.org/ml/gcc-cvs/2012-11/msg00176.html
>>> >
>>> > Thanks, K
>>>
>>> This caused:
>>>
>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55224
>>
>> Not only that, it also broke --enable-checking=yes,rtl bootstrap.
>> SET_DEST isn't valid on CALL, but XEXP (call, 0) is a MEM anyway and
>> the code looks for reg, so I think looking for CALL was just a mistake.
>>
>> This fixes the bootstrap, ok for trunk?
>>
>> 2012-11-06  Jakub Jelinek  <jakub@redhat.com>
>>
>>         * config/i386/i386.c (ix86_avx_u128_mode_after): Don't
>>         look for reg in CALL operand.
>>
>> --- gcc/config/i386/i386.c.jj   2012-11-06 18:10:22.000000000 +0100
>> +++ gcc/config/i386/i386.c      2012-11-06 20:15:09.068912242 +0100
>> @@ -15084,9 +15084,9 @@ ix86_avx_u128_mode_after (int mode, rtx
>>    /* Check for CALL instruction.  */
>>    if (CALL_P (insn))
>>      {
>> -      if (GET_CODE (pat) == SET || GET_CODE (pat) == CALL)
>> +      if (GET_CODE (pat) == SET)
>>         reg = SET_DEST (pat);
>> -      else if (GET_CODE (pat) ==  PARALLEL)
>> +      else if (GET_CODE (pat) == PARALLEL)
>>         for (i = XVECLEN (pat, 0) - 1; i >= 0; i--)
>>           {
>>             rtx x = XVECEXP (pat, 0, i);
>>
>>
>>         Jakub

Patch

--- gcc/config/i386/i386.c.jj	2012-11-06 18:10:22.000000000 +0100
+++ gcc/config/i386/i386.c	2012-11-06 20:15:09.068912242 +0100
@@ -15084,9 +15084,9 @@  ix86_avx_u128_mode_after (int mode, rtx
   /* Check for CALL instruction.  */
   if (CALL_P (insn))
     {
-      if (GET_CODE (pat) == SET || GET_CODE (pat) == CALL)
+      if (GET_CODE (pat) == SET)
 	reg = SET_DEST (pat);
-      else if (GET_CODE (pat) ==  PARALLEL)
+      else if (GET_CODE (pat) == PARALLEL)
 	for (i = XVECLEN (pat, 0) - 1; i >= 0; i--)
 	  {
 	    rtx x = XVECEXP (pat, 0, i);