From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4359 invoked by alias); 6 Aug 2011 10:47:14 -0000 Received: (qmail 4351 invoked by uid 22791); 6 Aug 2011 10:47:14 -0000 X-SWARE-Spam-Status: No, hits=-2.2 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,TW_AV,TW_VX,TW_XV X-Spam-Check-By: sourceware.org Received: from mail-pz0-f49.google.com (HELO mail-pz0-f49.google.com) (209.85.210.49) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 06 Aug 2011 10:46:59 +0000 Received: by pzk6 with SMTP id 6so5870298pzk.8 for ; Sat, 06 Aug 2011 03:46:59 -0700 (PDT) MIME-Version: 1.0 Received: by 10.142.165.10 with SMTP id n10mr2476569wfe.356.1312627619075; Sat, 06 Aug 2011 03:46:59 -0700 (PDT) Received: by 10.143.34.2 with HTTP; Sat, 6 Aug 2011 03:46:58 -0700 (PDT) In-Reply-To: References: Date: Sat, 06 Aug 2011 11:09:00 -0000 Message-ID: Subject: Re: [PATCH, testsuite, i386] AVX2 support for GCC From: Uros Bizjak To: Kirill Yukhin Cc: gcc-patches List , "H.J. Lu" Content-Type: text/plain; charset=ISO-8859-1 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2011-08/txt/msg00679.txt.bz2 On Thu, Aug 4, 2011 at 11:20 AM, Kirill Yukhin 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 "*andnot3" - [(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.