On Wed, Nov 7, 2012 at 9:04 AM, Jakub Jelinek <ja...@redhat.com> wrote:
> On Wed, Nov 07, 2012 at 08:08:08AM +0100, Uros Bizjak wrote:
>> >> 2012-11-06 Jakub Jelinek <ja...@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.
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;
}
