From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com [IPv6:2a00:1450:4864:20::52a]) by sourceware.org (Postfix) with ESMTPS id 632F13858023 for ; Wed, 1 Sep 2021 11:14:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 632F13858023 Received: by mail-ed1-x52a.google.com with SMTP id g21so3250154edw.4 for ; Wed, 01 Sep 2021 04:14:42 -0700 (PDT) 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=om2Q15iQvjbCPzgghfTcBsCf0taDyTvb1VH7tFsfU7s=; b=rcFh8X6Z7tMq6NVW+1TZ8uUHoSOe8aDVuXwHAefXWV5SNmeOAqDnXaBGqh8LqKUnQY eafl35YM91HTQ+VwAZOVJ3m8jniK1UwnXNN6g0J3Ni2QgZpoU0oXDtCt8WZOi0vKia/k /htEpoYerViemS83d7Mg6Ct4vGpcye2C+2G8yC6693CTfqEjo9eH6Hv6oXyLg5OO3Tat aZhXdVY3SuJW+Ey1YeTspJYNDh/PJoKCP/zXD4sPFYiPyT9xjiuleDGHuTT5K/Urknao 7Se4e8e8vqpBV9v+lqFmmAQZ+uw42i+eLgFFlj6g0+QT+8m5jgFtMQSPu/0RvcqI24dR IGcA== X-Gm-Message-State: AOAM530k5RLysZvc2rccqO/DSaBmwqg/aGS65Yy9VsseGKmXVmT8Jmgu TU+tqF2U8bhWEPw6i3yNnYc35/+Tn04bhP6c9cU= X-Google-Smtp-Source: ABdhPJxuvxxNEhliFq/Y/XROwstqMC8OU1wMqMlMy1rMoIwD4ThD2Di2Nzp0mcia6uOVEgQRIAmzVjecX/qsF3dkVWI= X-Received: by 2002:a50:c092:: with SMTP id k18mr34434705edf.361.1630494881236; Wed, 01 Sep 2021 04:14:41 -0700 (PDT) MIME-Version: 1.0 References: <20210827065259.5764-1-hongtao.liu@intel.com> In-Reply-To: From: Richard Biener Date: Wed, 1 Sep 2021 13:14:30 +0200 Message-ID: Subject: Re: [PATCH] Check the type of mask while generating cond_op in gimple simplication. To: Hongtao Liu Cc: liuhongt , Richard Sandiford , GCC Patches Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.6 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.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Wed, 01 Sep 2021 11:14:53 -0000 On Wed, Sep 1, 2021 at 8:28 AM Hongtao Liu wrote: > > On Tue, Aug 31, 2021 at 7:56 PM Richard Biener > wrote: > > > > On Tue, Aug 31, 2021 at 12:18 PM Hongtao Liu wrote: > > > > > > On Mon, Aug 30, 2021 at 8:25 PM Richard Biener via Gcc-patches > > > wrote: > > > > > > > > On Fri, Aug 27, 2021 at 8:53 AM liuhongt wrote: > > > > > > > > > > When gimple simplifcation try to combine op and vec_cond_expr to cond_op, > > > > > it doesn't check if mask type matches. It causes an ICE when expand cond_op > > > > > with mismatched mode. > > > > > This patch add a function named cond_vectorized_internal_fn_supported_p > > > > > to additionally check mask type than vectorized_internal_fn_supported_p. > > > > > > > > > > Bootstrapped and regtested on x86_64-linux-gnu{-m32,}. > > > > > Ok for trunk? > > > > > > > > > > gcc/ChangeLog: > > > > > > > > > > PR middle-end/102080 > > > > > * internal-fn.c (cond_vectorized_internal_fn_supported_p): New functions. > > > > > * internal-fn.h (cond_vectorized_internal_fn_supported_p): New declaration. > > > > > * match.pd: Check the type of mask while generating cond_op in > > > > > gimple simplication. > > > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > > > PR middle-end/102080 > > > > > * gcc.target/i386/pr102080.c: New test. > > > > > --- > > > > > gcc/internal-fn.c | 22 ++++++++++++++++++++++ > > > > > gcc/internal-fn.h | 1 + > > > > > gcc/match.pd | 24 ++++++++++++++++-------- > > > > > gcc/testsuite/gcc.target/i386/pr102080.c | 16 ++++++++++++++++ > > > > > 4 files changed, 55 insertions(+), 8 deletions(-) > > > > > create mode 100644 gcc/testsuite/gcc.target/i386/pr102080.c > > > > > > > > > > diff --git a/gcc/internal-fn.c b/gcc/internal-fn.c > > > > > index 1360a00f0b9..8b2b65db1a7 100644 > > > > > --- a/gcc/internal-fn.c > > > > > +++ b/gcc/internal-fn.c > > > > > @@ -4102,6 +4102,28 @@ expand_internal_call (gcall *stmt) > > > > > expand_internal_call (gimple_call_internal_fn (stmt), stmt); > > > > > } > > > > > > > > > > +/* Check cond_op for vector modes since vectorized_internal_fn_supported_p > > > > > + doesn't check if mask type matches. */ > > > > > +bool > > > > > +cond_vectorized_internal_fn_supported_p (internal_fn ifn, tree type, > > > > > + tree mask_type) > > > > > +{ > > > > > + if (!vectorized_internal_fn_supported_p (ifn, type)) > > > > > + return false; > > > > > + > > > > > + machine_mode mask_mode; > > > > > + machine_mode vmode = TYPE_MODE (type); > > > > > + int size1, size2; > > > > > + if (VECTOR_MODE_P (vmode) > > > > > + && targetm.vectorize.get_mask_mode (vmode).exists(&mask_mode) > > > > > + && GET_MODE_SIZE (mask_mode).is_constant (&size1) > > > > > + && GET_MODE_SIZE (TYPE_MODE (mask_type)).is_constant (&size2) > > > > > + && size1 != size2) > > > > > > > > Why do we check for equal size rather than just mode equality which > > > I originally thought TYPE_MODE of vector(8) was > > > not QImode, Changed the patch to check mode equality. > > > Update patch. > > > > Looking at all this it seems the match.pd patterns should have not > > used vectorized_internal_fn_supported_p but direct_internal_fn_supported_p > > which is equivalent here because we're always working with vector modes? > > > > And then shouldn't we look at the actual optab whether the mask mode matches > > the expectation rather than going around via the target hook which may not have > > enough context to decide which mask mode to use? > How about this? > > +/* Return true if target supports cond_op with data TYPE and > + mask MASK_TYPE. */ > +bool > +cond_internal_fn_supported_p (internal_fn ifn, tree type, > + tree mask_type) > +{ > + tree_pair types = tree_pair (type, type); > + optab tmp = direct_internal_fn_optab (ifn, types); > + machine_mode vmode = TYPE_MODE (type); > + insn_code icode = direct_optab_handler (tmp, vmode); > + if (icode == CODE_FOR_nothing) > + return false; > + > + machine_mode mask_mode = TYPE_MODE (mask_type); > + /* Can't create rtx and use insn_operand_matches here. */ > + return insn_data[icode].operand[0].mode == vmode > + && insn_data[icode].operand[1].mode == mask_mode; > +} > + Yeah, sth like that, though the operand[0].mode test should be redudnant. I think we should assert or have a whiltelist for the internal function we support to be queried this way. Not sure if we can directly access the 'cond_binary/cond_ternary' classification used in internal-fn.def, that would be best. Richard, what are your thoughts about all this? Thanks, Richard. > Update patch > > > > > In any case if the approach of the patch is correct shouldn't it do > > > > if (VECTOR_MODE_P (vmode) > > && (!targetm.vectorize.get_mask_mode (vmode).exists(&mask_mode) > > || mask_mode != TYPE_MODE (mask_type))) > > return false; > > > > that is, not return true if there's no mask mode for the data mode? > > > > Given the first observation should we call the function > > direct_cond_internal_fn_supported_p () instead and as to the second > > observation, look at the optab operands mode? > > > > Richard. > > > > > > I think would work for non-constant sized modes as well? And when > > > > using sizes you'd instead use maybe_ne (GET_MODE_SIZE (mask_mode), > > > > GET_MODE_SIZE (TYPE_MODE (mask_type))) > > > > > > > > Thanks, > > > > Richard. > > > > > > > > > + return false; > > > > > + > > > > > + return true; > > > > > +} > > > > > + > > > > > /* If TYPE is a vector type, return true if IFN is a direct internal > > > > > function that is supported for that type. If TYPE is a scalar type, > > > > > return true if IFN is a direct internal function that is supported for > > > > > diff --git a/gcc/internal-fn.h b/gcc/internal-fn.h > > > > > index 19d0f849a5a..f0aea00103c 100644 > > > > > --- a/gcc/internal-fn.h > > > > > +++ b/gcc/internal-fn.h > > > > > @@ -236,5 +236,6 @@ extern void expand_PHI (internal_fn, gcall *); > > > > > extern void expand_SHUFFLEVECTOR (internal_fn, gcall *); > > > > > > > > > > extern bool vectorized_internal_fn_supported_p (internal_fn, tree); > > > > > +extern bool cond_vectorized_internal_fn_supported_p (internal_fn, tree, tree); > > > > > > > > > > #endif > > > > > diff --git a/gcc/match.pd b/gcc/match.pd > > > > > index e5bbb123a6a..72b1bc674db 100644 > > > > > --- a/gcc/match.pd > > > > > +++ b/gcc/match.pd > > > > > @@ -6987,14 +6987,18 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) > > > > > cond_op (COND_BINARY) > > > > > (simplify > > > > > (vec_cond @0 (view_convert? (uncond_op@4 @1 @2)) @3) > > > > > - (with { tree op_type = TREE_TYPE (@4); } > > > > > - (if (vectorized_internal_fn_supported_p (as_internal_fn (cond_op), op_type) > > > > > + (with { tree op_type = TREE_TYPE (@4); > > > > > + tree mask_type = TREE_TYPE (@0); } > > > > > + (if (cond_vectorized_internal_fn_supported_p (as_internal_fn (cond_op), > > > > > + op_type, mask_type) > > > > > && element_precision (type) == element_precision (op_type)) > > > > > (view_convert (cond_op @0 @1 @2 (view_convert:op_type @3)))))) > > > > > (simplify > > > > > (vec_cond @0 @1 (view_convert? (uncond_op@4 @2 @3))) > > > > > - (with { tree op_type = TREE_TYPE (@4); } > > > > > - (if (vectorized_internal_fn_supported_p (as_internal_fn (cond_op), op_type) > > > > > + (with { tree op_type = TREE_TYPE (@4); > > > > > + tree mask_type = TREE_TYPE (@0); } > > > > > + (if (cond_vectorized_internal_fn_supported_p (as_internal_fn (cond_op), > > > > > + op_type, mask_type) > > > > > && element_precision (type) == element_precision (op_type)) > > > > > (view_convert (cond_op (bit_not @0) @2 @3 (view_convert:op_type @1))))))) > > > > > > > > > > @@ -7003,14 +7007,18 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) > > > > > cond_op (COND_TERNARY) > > > > > (simplify > > > > > (vec_cond @0 (view_convert? (uncond_op@5 @1 @2 @3)) @4) > > > > > - (with { tree op_type = TREE_TYPE (@5); } > > > > > - (if (vectorized_internal_fn_supported_p (as_internal_fn (cond_op), op_type) > > > > > + (with { tree op_type = TREE_TYPE (@5); > > > > > + tree mask_type = TREE_TYPE (@0); } > > > > > + (if (cond_vectorized_internal_fn_supported_p (as_internal_fn (cond_op), > > > > > + op_type, mask_type) > > > > > && element_precision (type) == element_precision (op_type)) > > > > > (view_convert (cond_op @0 @1 @2 @3 (view_convert:op_type @4)))))) > > > > > (simplify > > > > > (vec_cond @0 @1 (view_convert? (uncond_op@5 @2 @3 @4))) > > > > > - (with { tree op_type = TREE_TYPE (@5); } > > > > > - (if (vectorized_internal_fn_supported_p (as_internal_fn (cond_op), op_type) > > > > > + (with { tree op_type = TREE_TYPE (@5); > > > > > + tree mask_type = TREE_TYPE (@0); } > > > > > + (if (cond_vectorized_internal_fn_supported_p (as_internal_fn (cond_op), > > > > > + op_type, mask_type) > > > > > && element_precision (type) == element_precision (op_type)) > > > > > (view_convert (cond_op (bit_not @0) @2 @3 @4 > > > > > (view_convert:op_type @1))))))) > > > > > diff --git a/gcc/testsuite/gcc.target/i386/pr102080.c b/gcc/testsuite/gcc.target/i386/pr102080.c > > > > > new file mode 100644 > > > > > index 00000000000..6a40a75e1c5 > > > > > --- /dev/null > > > > > +++ b/gcc/testsuite/gcc.target/i386/pr102080.c > > > > > @@ -0,0 +1,16 @@ > > > > > +#include > > > > > +typedef float __m256 __attribute__((__vector_size__(32))); > > > > > +__m256 _mm256_blendv_ps___Y, _mm256_blendv_ps___M, _mm256_mul_ps___A, > > > > > + _mm256_mul_ps___B, IfThenElse___trans_tmp_9; > > > > > + > > > > > +void > > > > > +__attribute__ ((target("avx"))) > > > > > +IfThenElse (__m256 no) { > > > > > + IfThenElse___trans_tmp_9 = _mm256_blendv_ps (no, _mm256_blendv_ps___Y, _mm256_blendv_ps___M); > > > > > +} > > > > > +void > > > > > +__attribute__ ((target("avx512vl"))) > > > > > +EncodedFromDisplay() { > > > > > + __m256 __trans_tmp_11 = _mm256_mul_ps___A * _mm256_mul_ps___B; > > > > > + IfThenElse(__trans_tmp_11); > > > > > +} > > > > > -- > > > > > 2.18.1 > > > > > > > > > > > > > > > > > -- > > > BR, > > > Hongtao > > > > -- > BR, > Hongtao