Hi, here is patch for fixing FMA typo + bunch of traling spaces removed. Changelog entry:
2011-08-09 Kirill Yukhin <kirill.yuk...@intel.com>
* config/i386/i386.c: Remove traling spaces.
* config/i386/sse.md: Likewise.
(*fma_fmadd_<mode>): Update.
(*fma_fmsub_<mode>): Likewise.
(*fma_fnmadd_<mode>): Likewise.
(*fma_fnmsub_<mode>): Likewise.
Patch is attached. It is successfully bootstrapped.
Could anybody with `waa` commit it?
--
Thanks, K
On Sat, Aug 6, 2011 at 2:46 PM, Uros Bizjak <ubiz...@gmail.com> wrote:
> On Thu, Aug 4, 2011 at 11:20 AM, Kirill Yukhin <kirill.yuk...@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.
>
fma.typo.gcc.patch
Description: Binary data
