Re: [PATCH] Vzeroupper placement/47440

[Vladimir Yakovlev <vbyakovl23 at gmail dot com>]

Here is Changelogs and patch after fixing Uros remarks.

2012-11-04  Vladimir Yakovlev  <vladimir.b.yakovlev@intel.com>

        * mode-switching.c (create_pre_exit): Added code for
maybe_builtin_apply case.

        * config/i386/i386-protos.h (emit_i387_cw_initialization): Deleted.
        (emit_vzero): Added prototype.
        (ix86_mode_entry): Likewise.
        (ix86_mode_exit): Likewise.
        (ix86_emit_mode_set): Likewise.

        * config/i386/i386.c (typedef struct block_info_def): Deleted.
        (define BLOCK_INFO): Deleted.
        (check_avx256_stores): Added checking for MEM_P.
        (move_or_delete_vzeroupper_2): Deleted.
        (move_or_delete_vzeroupper_1): Deleted.
        (move_or_delete_vzeroupper): Deleted.
        (ix86_maybe_emit_epilogue_vzeroupper): Deleted.
        (function_pass_avx256_p): Deleted.
        (ix86_function_ok_for_sibcall): Deleted disabling sibcall.
        (nit_cumulative_args): Deleted initialization of of avx256 fields of
        cfun->machine.
        (ix86_emit_restore_sse_regs_using_mov): Deleted vzeroupper generation.
        (ix86_expand_epilogue): Likewise.
        (ix86_avx_u128_mode_needed): New.
        (ix86_i387_mode_needed): Renamed ix86_mode_needed.
        (ix86_mode_needed): New.
        (ix86_avx_u128_mode_after): New.
        (ix86_mode_after): New.
        (ix86_avx_u128_mode_entry): New.
        (ix86_mode_entry): New.
        (ix86_avx_u128_mode_exit): New.
        (ix86_mode_exit): New.
        (ix86_emit_mode_set): New.
        (ix86_expand_call): Deleted vzeroupper generation.
        (ix86_split_call_vzeroupper): Deleted.
        (ix86_init_machine_status): Initialzed optimize_mode_switching.
        (ix86_expand_special_args_builtin): Changed.
        (ix86_reorg): Deleted a call of move_or_delete_vzeroupper.

        * config/i386/i386.h  (VALID_AVX256_REG_OR_OI_MODE): New.
        (AVX_U128): New.
        (avx_u128_state): New.
        (NUM_MODES_FOR_MODE_SWITCHING): Added AVX_U128_ANY.
        (MODE_AFTER): New.
        (MODE_ENTRY): New.
        (MODE_EXIT): New.
        (EMIT_MODE_SET): Changed.
        (machine_function): Deleted avx256 fields.

        * config/i386/i386.md (UNSPEC_CALL_NEEDS_VZEROUPPER): Deleted.
        (define_insn_and_split "*call_vzeroupper"): Deleted.
        (define_insn_and_split "*call_rex64_ms_sysv_vzeroupper"): Deleted.
        (define_insn_and_split "*sibcall_vzeroupper"): Deleted.
        (define_insn_and_split "*call_pop_vzeroupper"): Deleted.
        (define_insn_and_split "*sibcall_pop_vzeroupper"): Deleted.
        (define_insn_and_split "*call_value_vzeroupper"): Deleted.
        (define_insn_and_split "*sibcall_value_vzeroupper"): Deleted.
        (define_insn_and_split "*call_value_rex64_ms_sysv_vzeroupper"): Deleted.
        (define_insn_and_split "*call_value_pop_vzeroupper"): Deleted.
        (define_insn_and_split "*sibcall_value_pop_vzeroupper"): Deleted.
        (define_expand "return"): Deleted vzeroupper emitting.
        (define_expand "simple_return"): Deleted.

        * config/i386/predicates.md (vzeroupper_operation): New.

        * config/i386/sse.md (avx_vzeroupper): Changed.

2012-11-04  Vladimir Yakovlev  <vladimir.b.yakovlev@intel.com>

        * gcc.target/i386/avx-vzeroupper-5.c: Changed scan-assembler-times.
        gcc.target/i386/avx-vzeroupper-8.c: Likewise.
        gcc.target/i386/avx-vzeroupper-9.c: Likewise.
        gcc.target/i386/avx-vzeroupper-10.c: Likewise.
        gcc.target/i386/avx-vzeroupper-11.c: Likewise.
        gcc.target/i386/avx-vzeroupper-12.c: Likewise.
        gcc.target/i386/avx-vzeroupper-19.c: Likewis.
        gcc.target/i386/avx-vzeroupper-27.c: New.

2012/11/4 Uros Bizjak <ubizjak@gmail.com>:
> On Sun, Nov 4, 2012 at 5:18 PM, Vladimir Yakovlev <vbyakovl23@gmail.com> wrote:
>> Thank you for review. I did changes you asked (see attached) with
>> small change: I left argument in 'emit_insn (GEN_FCN (icode)
>> (target));' because 'GEN_FCN (icode)' requeres it.
>
> Yes, you are correct.
>
> -/* Output code to initialize control word copies used by trunc?f?i and
> -   rounding patterns.  CURRENT_MODE is set to current control word,
> -   while NEW_MODE is set to new control word.  */
> -
>
> Please leave this comment...
>
> +#define VALID_AVX256_REG_OR_OI_MODE(m)                                 \
> +  (VALID_AVX256_REG_MODE (m)                                           \
> +   || (m) == OImode)
>
> Please use (MODE) as is case with other predicates. Also, put on one line:
>
> +  (VALID_AVX256_REG_MODE (MODE) || (MODE) == OImode)
>
> +;; eturn true if OP is a vzeroupper operation.
>
> Return ...
>
> +  [(unspec_volatile [(const_int 0)]
>                     UNSPECV_VZEROUPPER)]
>
> Please merge these two lines to one line.
>
>> Some comment about tests
>>> -/* { dg-final { scan-assembler-not "avx_vzeroupper" } } */
>>> +/* { dg-final { scan-assembler-times "avx_vzeroupper" 3 } } */
>>>
>>> (... and a couple of similar testsuite changes ...)
>>>
>>> These asm scans were put there for a reason. I assume you have looked
>>> at these differences and are correct (this also implies that current
>>> vzeroupper placement code is not optimal or even wrong).
>>>
>>
>> The tests use builtin functions therefore I don't remove them.
>
> Yes, I agree with this approach.
>
> These additional changes are OK, I have no further comments.
>
> Thanks,
> Uros.

Attachment: vzu.patch
Description: Binary data