From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 4781E3858C52 for ; Tue, 17 Oct 2023 13:36:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 4781E3858C52 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 4781E3858C52 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=217.140.110.172 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1697549763; cv=none; b=v/amokym3jq2tMnRvAHdgGAy4R4bqgbRzUPeUArfPZ1kITTpj63E8ala1sBLhqV+N2iJEWMu9TRDPwCGw+KIQb5lpMZ5eML1R8xmSWTOyUJCdgCwZQv6StKuKcMWpK0hQL8QtKzr4bnXLDyfi2gBAZBt3b5Zxm53HJF/YX+lGkM= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1697549763; c=relaxed/simple; bh=8mGNY+l/vKRoyiQ9eLYVzz3Cea0Cxpr1QfnBzzkbVhI=; h=From:To:Subject:Date:Message-ID:MIME-Version; b=LMqYqxQ+vDjXs/15jGB90QqbjSZuiXHcA8j/z/vDunRemQCMNZoO5pBbwSfXKrYrEJ9l43NJHp/kg1LH/0pWwKKcJ3Bufnybz3v3M7hweKmJ8WNNyYXY+XQy8peiwIf4iW9zZz/sJ5ngdhhUXoghHPw8lbC+Zmb8wFnIwnnQBqY= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 66BB52F4; Tue, 17 Oct 2023 06:36:40 -0700 (PDT) Received: from localhost (unknown [10.32.110.65]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 40E543F5A1; Tue, 17 Oct 2023 06:35:59 -0700 (PDT) From: Richard Sandiford To: Robin Dapp Mail-Followup-To: Robin Dapp ,Richard Biener , Robin Dapp via Gcc-patches , richard.sandiford@arm.com Cc: Richard Biener , Robin Dapp via Gcc-patches Subject: Re: [PATCH] gimple-match: Do not try UNCOND optimization with COND_LEN. References: <4b77e155-0936-67d6-ab2d-ae7ef49bfde0@gmail.com> <4afb967d-96ea-7e74-1a35-c86aa5a5ffa6@gmail.com> <38b16b69-1b82-420c-839b-d82278515f10@gmail.com> <03a8c49e-af19-4b38-966b-e9ddae4863f5@gmail.com> Date: Tue, 17 Oct 2023 14:35:58 +0100 In-Reply-To: <03a8c49e-af19-4b38-966b-e9ddae4863f5@gmail.com> (Robin Dapp's message of "Tue, 17 Oct 2023 13:39:39 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-24.1 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Robin Dapp writes: >>> I don't know much about valueisation either :) But it does feel >>> like we're working around the lack of a LEN form of COND_EXPR. >>> In other words, it seems odd that we can do: >>> >>> IFN_COND_LEN_ADD (mask, a, 0, b, len, bias) >>> >>> but we can't do: >>> >>> IFN_COND_LEN (mask, a, b, len, bias) >>> >>> There seems to be no way of applying a length without also finding an >>> operation to perform. >> >> Indeed .. maybe - _maybe_ we want to scrap VEC_COND_EXPR for >> IFN_COND{,_LEN} to be more consistent here? > > So, yes we could define IFN_COND_LEN (or VCOND_MASK_LEN) but I'd > assume that there would be a whole lot of follow-up things to > consider. > > I'm wondering if we really gain something from the the round-trip > via VEC_COND_EXPR when we eventually create a COND_(LEN_)_OP anyway? The main purpose of the VEC_COND_EXPR isn't as an intermediate step, but as an end in its own right. E.g. it allows: IFN_COND_ADD (mask, cst1, cst2, else) to be folded to: VEC_COND_EXPR This is especially useful when vectorisation has the effect of completely unrolling a loop. The VEC_COND_EXPR is only used if the equivalent unconditional rule folds to a gimple value. > Sure, if the target doesn't have the particular operation we would > want a VEC_COND_EXPR. Same if SEQ is somehow more complicated. > > So the IFN_COND(_LEN) =? VCOND_MASK(_LEN) discussion notwithstanding, > couldn't what I naively proposed be helpful as well? I don't think it's independently useful, since the fold that it's attempting is one that match.pd should be able to do. match.pd can also do it in a more general way, since it isn't restricted to looking at the currenct sequence. > Or do we > potentially lose optimizations during the time where e.g. a > _foo = a BINOP b > VEC_COND_EXPR (cond, foo, else) > has not yet been converted into a > COND_OP? Yeah, it would miss out on that too. > We already create COND_OPs for the other paths > (via convert_conditional_op) so why not for this one? Or am I missing > some interdependence with SEQ? The purpose of this code is to see what happens if we apply the usual folds for unconditional ops to the corresponding conditional forms. E.g. for IFN_COND_ADD (mask, a, b, c) it sees what a + b would fold to, then tries to reapply the VEC_DOND_EXPR (mask, ..., c) to the result. If a + b folds to a gimple value, we can fold to a VEC_COND_EXPR involving that gimple value, as discussed above. This could happen if a + b folds to a constant, or for things like a + 0 -> a. If instead a + b folds to a new operation (say a + b' or a - b'), we need to construct the equivalent conditional form of that operation, with the same mask and else values. This is a correctness issue rather than an optimisation. As the comment in: /* Otherwise try rewriting the operation as an IFN_COND_* call. Again, this isn't a simplification in itself, since it's what RES_OP already described. */ if (convert_conditional_op (res_op, &new_op)) *res_op = new_op; says, it's just reconstituting what RES_OP describes in gimple form. If that isn't possible then the simplification must fail. In some cases we could, as a follow-on, try to make a a' op b' fold result fall back to an unconditional a' op b' followed by a VEC_COND_EXPR. But we don't do that currently. It isn't safe in all cases, since IFN_COND_ADD only adds active elements, whereas an unconditional a' op b' would operate on all elements. I also don't know of any specific example where this would be useful on SVE. Thanks, Richard > > FWIW I did a full bootstrap and testsuite run on the usual architectures > showing no changes with the attached patch. > > Regards > Robin > > Subject: [PATCH] gimple-match: Create COND_OP directly if possible. > > This patch converts simplified sequences into conditional operations > instead of VEC_COND_EXPRs if the target supports them. > This helps for len-masked targets which cannot directly use a > VEC_COND_EXPR in the presence of length masking. > > gcc/ChangeLog: > > * gimple-match-exports.cc (directly_supported_p): Define. > (maybe_resimplify_conditional_op): Create COND_OP directly. > * gimple-match.h (gimple_match_cond::gimple_match_cond): > Initialize length and bias. > --- > gcc/gimple-match-exports.cc | 40 ++++++++++++++++++++++++++++--------- > gcc/gimple-match.h | 7 +++++-- > 2 files changed, 36 insertions(+), 11 deletions(-) > > diff --git a/gcc/gimple-match-exports.cc b/gcc/gimple-match-exports.cc > index b36027b0bad..ba3bd1450db 100644 > --- a/gcc/gimple-match-exports.cc > +++ b/gcc/gimple-match-exports.cc > @@ -98,6 +98,8 @@ static bool gimple_resimplify5 (gimple_seq *, gimple_match_op *, tree (*)(tree)) > static bool gimple_resimplify6 (gimple_seq *, gimple_match_op *, tree (*)(tree)); > static bool gimple_resimplify7 (gimple_seq *, gimple_match_op *, tree (*)(tree)); > > +bool directly_supported_p (code_helper, tree, optab_subtype); > + > /* Match and simplify the toplevel valueized operation THIS. > Replaces THIS with a simplified and/or canonicalized result and > returns whether any change was made. */ > @@ -299,22 +301,42 @@ maybe_resimplify_conditional_op (gimple_seq *seq, gimple_match_op *res_op, > } > } > > - /* If the "then" value is a gimple value and the "else" value matters, > - create a VEC_COND_EXPR between them, then see if it can be further > - simplified. */ > + /* If the condition represents MASK ? THEN : ELSE, where THEN is a gimple > + value and ELSE matters, create a VEC_COND_EXPR between them, then see > + if it can be further simplified. > + For COND_LEN masking, try to create a COND_LEN_OP directly in case > + SEQ contains a supportable operation. */ > gimple_match_op new_op; > if (res_op->cond.else_value > && VECTOR_TYPE_P (res_op->type) > && gimple_simplified_result_is_gimple_val (res_op)) > { > - new_op.set_op (VEC_COND_EXPR, res_op->type, > - res_op->cond.cond, res_op->ops[0], > - res_op->cond.else_value); > - *res_op = new_op; > - return gimple_resimplify3 (seq, res_op, valueize); > + /* If a previous simplification was pushed to SEQ > + and we can convert it to a COND_OP directly, do so > + in order to save a round-trip via VEC_COND_EXPR -> COND_OP. */ > + if (seq && *seq && is_gimple_assign (*seq) > + && directly_supported_p (gimple_assign_rhs_code (*seq), res_op->type, > + optab_scalar)) > + { > + res_op->code = gimple_assign_rhs_code (*seq); > + res_op->num_ops = gimple_num_ops (*seq) - 1; > + res_op->ops[0] = gimple_assign_rhs1 (*seq); > + if (res_op->num_ops > 1) > + res_op->ops[1] = gimple_assign_rhs2 (*seq); > + if (res_op->num_ops > 2) > + res_op->ops[2] = gimple_assign_rhs2 (*seq); > + } > + else if (!res_op->cond.len) > + { > + new_op.set_op (VEC_COND_EXPR, res_op->type, > + res_op->cond.cond, res_op->ops[0], > + res_op->cond.else_value); > + *res_op = new_op; > + return gimple_resimplify3 (seq, res_op, valueize); > + } > } > > - /* Otherwise try rewriting the operation as an IFN_COND_* call. > + /* Otherwise try rewriting the operation as an IFN_COND_(LEN_)* call. > Again, this isn't a simplification in itself, since it's what > RES_OP already described. */ > if (convert_conditional_op (res_op, &new_op)) > diff --git a/gcc/gimple-match.h b/gcc/gimple-match.h > index bec3ff42e3e..55c771d560f 100644 > --- a/gcc/gimple-match.h > +++ b/gcc/gimple-match.h > @@ -32,7 +32,9 @@ public: > enum uncond { UNCOND }; > > /* Build an unconditional op. */ > - gimple_match_cond (uncond) : cond (NULL_TREE), else_value (NULL_TREE) {} > + gimple_match_cond (uncond) > + : cond (NULL_TREE), else_value (NULL_TREE), len (NULL_TREE), > + bias (NULL_TREE) {} > gimple_match_cond (tree, tree); > gimple_match_cond (tree, tree, tree, tree); > > @@ -56,7 +58,8 @@ public: > > inline > gimple_match_cond::gimple_match_cond (tree cond_in, tree else_value_in) > - : cond (cond_in), else_value (else_value_in) > + : cond (cond_in), else_value (else_value_in), len (NULL_TREE), > + bias (NULL_TREE) > { > }