Re: [PATCH, testsuite, i386] AVX2 support for GCC

[Kirill Yukhin <kirill dot yukhin at gmail dot com>]

Hi,
Here is second stage patch.
It introduces AVX2 option, define etc.

ChangeLog entry:

2011-08-09  Kirill Yukhin  <kirill.yukhin@intel.com>

        * common/config/i386/i386-common.c (OPTION_MASK_ISA_AVX2_SET): New.
        (OPTION_MASK_ISA_FMA_UNSET): Update.
        (OPTION_MASK_ISA_AVX2_UNSET): New.
        (ix86_handle_option): Handle OPT_mavx2 case.
        * config/i386/cpuid.h (bit_AVX2): New.
        * config/i386/driver-i386.c (host_detect_local_cpu): Detect
        AVX2 feature.
        * config/i386/i386-c.c (ix86_target_macros_internal): Define
        __AVX2_ if needed.
        * config/i386/i386.c (ix86_option_override_internal): Handle
        AVX2 option, define new processor alias - "core-avx2".
        (ix86_valid_target_attribute_inner_p): Likewise.
        * config/i386/i386.h (TARGET_AVX2): New.
        * config/i386/i386.opt (mavx2): New.
        * doc/invoke.texi: Document -mavx2.

Patch is attached.
With workaround (attached):
  - bootstrapped
  - make-checked with no new fails.

Is it OK for trunk, after int64 is comitted?

Thanks, K

On Sat, Aug 6, 2011 at 2:46 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Thu, Aug 4, 2011 at 11:20 AM, Kirill Yukhin <kirill.yukhin@gmail.com> wrote:
>> During last few months I was working on AVX2 support for GCC.
>>
>> Here is a patch which conforms (hopefully) to Spec which can be found at [1]
>
> Whoa, mega-patch for review. This will be attacked in stages.
>
> 1. Typo fixes to fma_ patterns (and whitespace fixes):
>
> Please split these out to separate patch. These are OK, please commit
> them to SVN. ?You can also sneak whitespace fixes in this patch ...
>
> 2. Options and flags (including driver-i386.c):
>
> These are waiting for int64 patch. Please split these out,
> options/flags handling will be committed first, so TARGET_AVX2 is
> handled correctly.
>
> ATM, you don't need to change
>
> -int ix86_isa_flags = TARGET_64BIT_DEFAULT | TARGET_SUBTARGET_ISA_DEFAULT
> +long long int ix86_isa_flags = TARGET_64BIT_DEFAULT |
> TARGET_SUBTARGET_ISA_DEFAULT
>
> 3. Testsuite changes:
>
> Please split testsuite changes to separate patch... This patch will be
> committed last.
>
> 4. Main patch:
>
> Review, round 1...
>
> avx2intrin.h and corresponding intrinsic handling looks OK to me.
> However, you should add -mavx2 to following testsuite files that check
> usage of intrinsics:
>
> gcc.target/i386/sse-12.c (you did already)
> gcc.target/i386/sse-13.c
> gcc.target/i386/sse-14.c
> gcc.target/i386/sse-22.c
> gcc.target/i386/sse-23.c
>
> g++.dg/other/i386-2.C
> g++.dg/other/i386-3.C
>
> If your patch survives this torture, you get automatic approval on new
> headers and intrinsics handling stuff. ;)
>
> sse.md:
>
> The "interesting" stuff...
>
> As a general advice, do not introduce new mode attributes unless
> *really* necessary. Extend existing attributes instead. They don't
> need to match exactly the particular mode iterator, it is only
> important that all modes handled through mode attribute are defined.
>
> +(define_mode_attr avx2modesuffix
> + ?[(SF "ss") (DF "sd")
> + ? (V8SF "ps") (V4DF "pd")
> + ? (V4SF "ps") (V2DF "pd")
> + ? (V16QI "b") (V8HI "w") (V4SI "d") (V2DI "q")
> + ? (V8SI "d") (V4DI "q")])
>
> Add new modes to ssemodesuffix mode iterator and use it instead (it
> was just fixed to get rid of V8SI mode, defined to "si").
>
> +(define_mode_attr sse_avx2
> + ?[(V16QI "") (V32QI "avx2_")
> + ? (V8HI "") (V16HI "avx2_")
> + ? (V4SI "") (V8SI "avx2_")
> + ? (V2DI "") (V4DI "avx2_")])
>
> Remove. Not used anywhere.
>
> +(define_code_iterator lshift [lshiftrt ashift])
> +(define_code_attr lshift_insn [(lshiftrt "srl") (ashift "sll")])
> +(define_code_attr lshift [(lshiftrt "lshr") (ashift "lshl")])
>
> Missing comments, see i386.md.
>
> +(define_mode_attr SSESHORTMODE
> + ?[(V4DI "V4SI") (V2DI "V2SI")])
>
> ssehalfmode
>
> +(define_mode_attr SSELONGMODE
> + ?[(V16HI "V16SI") (V8HI "V8SI")])
>
> ssedoublemode
>
> +(define_mode_attr SSEBYTEMODE
> + ?[(V4DI "V32QI") (V2DI "V16QI")])
>
> ssebytemode
>
> +(define_mode_attr AVXTOSSEMODE
> + ?[(V4DI "V2DI") (V2DI "V2DI")
> + ? (V8SI "V4SI") (V4SI "V4SI")
> + ? (V16HI "V8HI") (V8HI "V8HI")
> + ? (V32QI "V16QI") (V16QI "V16QI")])
>
> Move near avx2_pbroadcast pattern.
>
> +(define_mode_attr shortmode
> + ?[(V4DI "v4si") (V2DI "v2si")])
>
> Not needed, you have wrong mode iterator choice for mul insns.
>
> +;; 32 byte integral vector modes handled by AVX
> +(define_mode_iterator AVX256MODEI [V32QI V16HI V8SI V4DI])
>
> Please be consistent with existing names/comments:
>
> ;; All 256bit vector integer modes
> (define_mode_iterator VF_256
> ?[...])
>
> ?;; Mix-n-match
> ?(define_mode_iterator AVX256MODE2P [V8SI V8SF V4DF])
> +(define_mode_iterator AVX256MODE124 [V32QI V16HI V8SI])
> +(define_mode_iterator AVX256MODE1248 [V32QI V16HI V8SI V4DI])
> +(define_mode_iterator AVX256MODE248 [V16HI V8SI V4DI])
>
> I am trying to remove this section! ?Please move these definitions to
> new section (under "Random 128bit ...")
>
> ;; Random 256bit vector integer mode combinations
> define_mode_iterator VI124_256 [...])
> ...etc.
>
> +;; For gather* insn patterns
> +(define_mode_iterator AVXMODE48P_SI
> + ? ? ? ? ? ? ? ? ? ? [V2DI V2DF V4DI V4DF V4SI V4SF V8SI V8SF])
> ...
>
> Please rename these to something like VEC_GATHER_MODE and put these
> iterators/attributes at the top of gather* patterns.
>
> +(define_mode_attr avx2
> + ?[(V32QI "avx2_") (V16HI "avx2_") (V8SI "avx2_") (V4DI "avx2_")
> + ? (V16QI "") (V8HI "") (V4SI "") (V2DI "")])
>
> Remove. Dupe with sse_avx2 mode iterator, and not used anywhere anyway.
>
> +;; Mapping from integer vector mode to mnemonic suffix
> +(define_mode_attr ssevecsize
> + ?[(V16QI "b") (V8HI "w") (V4SI "d") (V2DI "q")
> + ? (V32QI "b") (V16HI "w") (V8SI "d") (V4DI "q")])
>
> Use ssemodesuffix.
>
> +;; Mapping of vector modes to a vector mode of double size
> +(define_mode_attr ssedoublesizemode
> + ?[(V2DF "V4DF") (V2DI "V4DI") (V4SF "V8SF") (V4SI "V8SI")
> + ? (V8HI "V16HI") (V16QI "V32QI")
> + ? (V4DF "V8DF") (V8SF "V16SF")
> + ? (V4DI "V8DI") (V8SI "V16SI") (V16HI "V32HI") (V32QI "V64QI")])
>
> Remove and use ssedoublevecmode mode iterator instead.
>
> +;; Mapping for AVX
> +(define_mode_attr avxvecmode
> + ?[(V16QI "TI") (V8HI "TI") (V4SI "TI") (V2DI "TI") (V1TI "TI") (TI "TI")
> + ? (V4SF "V4SF") (V8SF "V8SF") (V2DF "V2DF") (V4DF "V4DF")
> + ? (V32QI "OI") (V16HI "OI") (V8SI "OI") (V4DI "OI")])
>
> Add new modes to sseinsnmode and use it instead.
>
> +(define_mode_attr avxvecsize [(V32QI "b") (V16HI "w") (V8SI "d") (V4DI "q")])
>
> Use ssemodesuffix instead.
>
> +(define_mode_attr avxhalfvecmode
> + ?[(V32QI "V16QI") (V16HI "V8HI") (V8SI "V4SI") (V4DI "V2DI")
> + ? (V8SF "V4SF") (V4DF "V2DF")
> + ? (V16QI ?"V8QI") (V8HI ?"V4HI") (V4SI "V2SI") (V4SF "V2SF")])
>
> Use ssehalfvecmode instead.
>
> +(define_mode_attr avxscalarmode
> + ?[(V16QI "QI") (V8HI ?"HI") (V4SI "SI") (V2DI "DI") (V4SF "SF") (V2DF "DF")
> + ? (V32QI "QI") (V16HI "HI") (V8SI "SI") (V4DI "DI") (V8SF "SF") (V4DF "DF")])
>
> Use ssescalarmode instead.
>
> +(define_mode_attr avxpermvecmode
> + ?[(V2DF "V2DI") (V4SF "V4SI") (V4DF "V4DI") (V8SF "V8SI")
> + ? (V8SI "V8SI") (V4DI "V4DI") (V4SI "V4SI") (V2DI "V2DI")])
>
> Add missing modes to sseintvecmode mode iterator and use it instead.
>
> +(define_mode_attr avxmodesuffixp
> + [(V2DF "pd") (V4SI "si") (V4SF "ps") (V8SF "ps") (V8SI "si")
> + ?(V4DF "pd")])
>
> Remove, not needed.
>
> +(define_mode_attr avxmodesuffix
> + ?[(V16QI "") (V32QI "256") (V4SI "") (V4SF "") (V2DF "") (V2DI "")
> + ? (V8SI "256") (V8SF "256") (V4DF "256") (V4DI "256")])
>
> Use avxsizesuffix instead.
>
> ?(define_insn "*andnot<mode>3"
> - ?[(set (match_operand:VI 0 "register_operand" "=x,x")
> - ? ? ? (and:VI
> - ? ? ? ? (not:VI (match_operand:VI 1 "register_operand" "0,x"))
> - ? ? ? ? (match_operand:VI 2 "nonimmediate_operand" "xm,xm")))]
> + ?[(set (match_operand:VI_AVX2 0 "register_operand" "=x,x")
> + ? ? ? (and:VI_AVX2
> + ? ? ? ? (not:VI_AVX2 (match_operand:VI_AVX2 1 "register_operand" "0,x"))
> + ? ? ? ? (match_operand:VI_AVX2 2 "nonimmediate_operand" "xm,xm")))]
>
> No! This pattern handles 256 bit VI modes through "...ps" AVX insn,
> you limited 256 bit modes to AVX2. Please change insn attributes
> instead. Other changes from VI -> VI_AVX2 are also wrong.
>
> Please repost patches (splitted to mode handling, testsuite and main
> patch), with requested changes for next review round.
>
> Thanks,
> Uros.
>

Attachment: avx2-3.gcc.patch.source.opts
Description: Binary data

Attachment: opt64.tmp.gcc.patch
Description: Binary data