From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x633.google.com (mail-ej1-x633.google.com [IPv6:2a00:1450:4864:20::633]) by sourceware.org (Postfix) with ESMTPS id AD94E384A00F for ; Thu, 12 Nov 2020 13:59:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org AD94E384A00F Received: by mail-ej1-x633.google.com with SMTP id za3so7854048ejb.5 for ; Thu, 12 Nov 2020 05:59:45 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=EwSVskptyfaN886BQ0si80JPKIYyUVJ+yZ+KsR9LBMI=; b=l/3IMsaAOm/vuaS+ukDd+mXp50qu8aGPPKteW3nRRplIHJ+UgkbuXdl1I3+08hTsN2 xJ2tBB58qlxpKom15kGXFJk93l6+ts/JKGpWf/cxN96NQUewioosLbeRrRJ1DAxqFRGG 9Xh0u/U+EvHwbmUmgu5wG0oCkZ+d6IZdKjjyYD9tXxwv2EBnIOkGFNYh53uFbZdF/sfc uy8Ra9XVrP4X82lScYpi4DPynzvHemyHUZ5meuHG6FVj9aYi1MGxXLlKmO/TMRM4Nmc4 T8PiRcZYiYLWf8/uzfeB2Z6t8A6UU4gkUubvCAHwLQnZLvAroxVgCQS3QrGALW9zBCc5 wx5Q== X-Gm-Message-State: AOAM533RT96MhG8fbcXlDgYYMmAFJWFxTWnBywO6rtD2UiXTfrdx+7Vn /wp5WeiPWjdRgH8a45yaCID5OSFSdRDGUjxts1Q= X-Google-Smtp-Source: ABdhPJwunjE8wQxtA6h6zzQWgKH7G1QmxN/xEo46b2KjA7WcLjFstG77sXZORdYTUPFsRlAB6v/MisoevpJ+3uib4Nw= X-Received: by 2002:a17:906:3acd:: with SMTP id z13mr31120527ejd.118.1605189584697; Thu, 12 Nov 2020 05:59:44 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Richard Biener Date: Thu, 12 Nov 2020 14:59:33 +0100 Message-ID: Subject: Re: [PATCH] [PR target/97194] [AVX2] Support variable index vec_set. To: Hongtao Liu Cc: Uros Bizjak , "gcc-patches@gcc.gnu.org" Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-9.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 12 Nov 2020 13:59:48 -0000 On Thu, Nov 12, 2020 at 10:23 AM Hongtao Liu wrote: > > On Thu, Nov 12, 2020 at 5:15 PM Hongtao Liu wrote: > > > > On Thu, Nov 12, 2020 at 5:12 PM Hongtao Liu wrote: > > > > > > On Thu, Nov 12, 2020 at 4:21 PM Uros Bizjak wrote: > > > > > > > > On Thu, Nov 12, 2020 at 3:04 AM Hongtao Liu wrote: > > > > > > > > > > > gcc/ChangeLog: > > > > > > > > > > > > > > PR target/97194 > > > > > > > * config/i386/i386-expand.c (ix86_expand_vector_set_var): New function. > > > > > > > * config/i386/i386-protos.h (ix86_expand_vector_set_var): New Decl. > > > > > > > * config/i386/predicates.md (vec_setm_operand): New predicate, > > > > > > > true for const_int_operand or register_operand under TARGET_AVX2. > > > > > > > * config/i386/sse.md (vec_set): Support both constant > > > > > > > and variable index vec_set. > > > > > > > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > > > > > > > * gcc.target/i386/avx2-vec-set-1.c: New test. > > > > > > > * gcc.target/i386/avx2-vec-set-2.c: New test. > > > > > > > * gcc.target/i386/avx512bw-vec-set-1.c: New test. > > > > > > > * gcc.target/i386/avx512bw-vec-set-2.c: New test. > > > > > > > * gcc.target/i386/avx512f-vec-set-2.c: New test. > > > > > > > * gcc.target/i386/avx512vl-vec-set-2.c: New test. > > > > > > > > > > > > +;; True for registers, or const_int_operand, used to vec_setm expander. > > > > > > +(define_predicate "vec_setm_operand" > > > > > > + (ior (and (match_operand 0 "register_operand") > > > > > > + (match_test "TARGET_AVX2")) > > > > > > + (match_code "const_int"))) > > > > > > + > > > > > > ;; True for registers, or 1 or -1. Used to optimize double-word shifts. > > > > > > (define_predicate "reg_or_pm1_operand" > > > > > > (ior (match_operand 0 "register_operand") > > > > > > diff --git a/gcc/config/i386/sse.md b/gcc/config/i386/sse.md > > > > > > index b153a87fb98..1798e5dea75 100644 > > > > > > --- a/gcc/config/i386/sse.md > > > > > > +++ b/gcc/config/i386/sse.md > > > > > > @@ -8098,11 +8098,14 @@ (define_insn "vec_setv2df_0" > > > > > > (define_expand "vec_set" > > > > > > [(match_operand:V 0 "register_operand") > > > > > > (match_operand: 1 "register_operand") > > > > > > - (match_operand 2 "const_int_operand")] > > > > > > + (match_operand 2 "vec_setm_operand")] > > > > > > > > > > > > You need to specify a mode, otherwise a register of any mode can pass here. > > > > > > > > > > > Yes, theoretically, we only accept integer types. But in can_vec_set_var_idx_p > > > > > cut > > > > > --- > > > > > bool > > > > > can_vec_set_var_idx_p (machine_mode vec_mode) > > > > > { > > > > > if (!VECTOR_MODE_P (vec_mode)) > > > > > return false; > > > > > > > > > > machine_mode inner_mode = GET_MODE_INNER (vec_mode); > > > > > rtx reg1 = alloca_raw_REG (vec_mode, LAST_VIRTUAL_REGISTER + 1); > > > > > rtx reg2 = alloca_raw_REG (inner_mode, LAST_VIRTUAL_REGISTER + 2); > > > > > rtx reg3 = alloca_raw_REG (VOIDmode, LAST_VIRTUAL_REGISTER + 3); > > > > > > > > > > enum insn_code icode = optab_handler (vec_set_optab, vec_mode); > > > > > > > > > > return icode != CODE_FOR_nothing && insn_operand_matches (icode, 0, reg1) > > > > > && insn_operand_matches (icode, 1, reg2) > > > > > && insn_operand_matches (icode, 2, reg3); > > > > > } > > > > > --- > > > > > > > > > > reg3 is assumed to be VOIDmode, set anymode in match_operand 2 will > > > > > fail insn_operand_matches (icode, 2, reg3) > > > > > --- > > > > > (gdb) p insn_operand_matches(icode,2,reg3) > > > > > $5 = false > > > > > (gdb) > > > > > --- > > > > > > > > > > Maybe we need to change > > > > > > > > > > rtx reg3 = alloca_raw_REG (VOIDmode, LAST_VIRTUAL_REGISTER + 3); > > > > > > > > > > to > > > > > > > > > > rtx reg3 = alloca_raw_REG (SImode, LAST_VIRTUAL_REGISTER + 3); > > > > > > > > > > cc Richard Biener, any thoughts? > > > > > > > > There are two targets (gcn in gcn-valu.md and s390 in vector.md) that > > > > specify SImode for operand 2 in vec_setM pattern and allow register > > > > operands. I wonder if and how they manage to generate the pattern. > > > > > > > > Uros. > > > > > > Variable index vec_set is enabled by r11-3486, about two months ago in > > > [1]. But for the upper two targets, the codes are already there since > > > GCC10(maybe earlier, i just looked at gcc10 branch), I don't think > > > those codes are for [1]. > > > > > > [1] https://gcc.gnu.org/pipermail/gcc-patches/2020-October/555905.html > > > > > > > > > -- > > > BR, > > > Hongtao > > > > Correct [1] https://gcc.gnu.org/pipermail/gcc-patches/2020-September/554240.html > > > > -- > > BR, > > Hongtao > > in https://gcc.gnu.org/pipermail/gcc-patches/2020-September/554592.html > > It says > > > >> +can_vec_set_var_idx_p (enum tree_code code, machine_mode vec_mode, > > >> + machine_mode value_mode, machine_mode idx_mode) > > > > > > toplevel comment missing > > > > > >> +{ > > >> + gcc_assert (code == VECTOR_TYPE); > > > > > > what's the point of pasing 'code' here then? Since the optab only has a single > > > mode, the vector mode, the value_mode is redundant as well. And I guess > > > we might want to handle "arbitrary" index modes? That is, the .md expanders > > > should not restrict its mode - I guess it simply uses VOIDmode at the moment > > > (for integer constants). Not sure how to best do this without an explicit mode > > > in the optab ... > > > > Yes, removed 'code' and value_mode by checking VECTOR_MODE_P and use GET_MODE_INNER > > for value_mode. ".md expanders" shall support for integer constants index mode, but > > I guess they shouldn't be expanded by IFN as this function is for variable index > > insert only? Anyway, the v3 patch used VOIDmode check... I'm not sure what best to do here, as said accepting "any" (integer) mode as input is desirable (SImode, DImode but eventually also smaller modes). How that can be best achieved I don't know. Why's not specifying any mode in the patter no good? Just make sure you appropriately extend/subreg it? We can make sure it will be an integer mode in the expander itself. Richard. > > -- > BR, > Hongtao