diff mbox

Vzeroupper placement/47440

Message ID CAFULd4bEjUnnEkSXb=6zEaiUjDQ4foUTTJa23ycocmqtQZabDw@mail.gmail.com
State New
Headers show

Commit Message

Uros Bizjak Nov. 7, 2012, 9:41 a.m. UTC 
On Wed, Nov 7, 2012 at 9:04 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> 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.

Indeed, I didn't notice reverse 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.

You are right, I am testing the attached patch.

> 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 have some doubts that this function is fully correct. The comment says:

/* Check if a 256bit AVX register is referenced in stores.   */

But, we are in fact checking stores and uses.

IIRC, U128_DIRTY state is only set for stores to YMM register, so:

static void
check_avx256_stores (rtx dest, const_rtx set ATTRIBUTE_UNUSED, void *data)
{
  if (REG_P (dest)
      && VALID_AVX256_REG_OR_OI_MODE (GET_MODE (dest)))
    {
      enum upper_128bits_state *state
	= (enum upper_128bits_state *) data;
      *state = used;
    }
}

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

Yes, Vladimir, can you please comment on these issues?

Uros.
diff mbox

Patch

Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c	(revision 193280)
+++ config/i386/i386.c	(working copy)
@@ -15079,30 +15079,8 @@ 
 ix86_avx_u128_mode_after (int mode, rtx insn)
 {
   rtx pat = PATTERN (insn);
-  rtx reg = NULL;
-  int i;
   enum upper_128bits_state state;
 
-  /* Check for CALL instruction.  */
-  if (CALL_P (insn))
-    {
-      if (GET_CODE (pat) == SET || GET_CODE (pat) == CALL)
-	reg = SET_DEST (pat);
-      else if (GET_CODE (pat) ==  PARALLEL)
-	for (i = XVECLEN (pat, 0) - 1; i >= 0; i--)
-	  {
-	    rtx x = XVECEXP (pat, 0, i);
-	    if (GET_CODE(x) == SET)
-	      reg = SET_DEST (x);
-	  }
-      /* Mode after call is set to AVX_U128_DIRTY if there are
-	 256bit modes used in the function return register.  */
-      if (reg && REG_P (reg) && VALID_AVX256_REG_OR_OI_MODE (GET_MODE (reg)))
-	return AVX_U128_DIRTY;
-      else
-	return AVX_U128_CLEAN;
-    }
-
   if (vzeroupper_operation (pat, VOIDmode)
       || vzeroall_operation (pat, VOIDmode))
     return AVX_U128_CLEAN;
@@ -15112,6 +15090,10 @@ 
   note_stores (pat, check_avx256_stores, &state);
   if (state == used)
     return AVX_U128_DIRTY;
+  /* We know that state is clean after CALL insn if there are no
+     256bit modes used in the function return register.  */
+  else if (CALL_P (insn) && state == unused)
+    return AVX_U128_CLEAN;
 
   return mode;
 }