From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28893 invoked by alias); 20 Dec 2017 00:27:58 -0000 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 Received: (qmail 28882 invoked by uid 89); 20 Dec 2017 00:27:57 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 20 Dec 2017 00:27:55 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8886F82100; Wed, 20 Dec 2017 00:27:54 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-2.rdu2.redhat.com [10.10.112.2]) by smtp.corp.redhat.com (Postfix) with ESMTP id 38BCD18AD6; Wed, 20 Dec 2017 00:27:53 +0000 (UTC) Subject: Re: [14/nn] Add helpers for shift count modes To: Richard Biener , GCC Patches , richard.sandiford@linaro.org References: <87wp3mxgir.fsf@linaro.org> <878tg2w1fv.fsf@linaro.org> <87mv3g3bqv.fsf@linaro.org> <87d13gq0ib.fsf@linaro.org> <87r2rwnhqw.fsf@linaro.org> <87vah24jkw.fsf@linaro.org> From: Jeff Law Message-ID: <3b2a13de-0614-e16d-ab85-34b96cfdc560@redhat.com> Date: Wed, 20 Dec 2017 00:27:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <87vah24jkw.fsf@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2017-12/txt/msg01327.txt.bz2 On 12/19/2017 12:13 PM, Richard Sandiford wrote: > Richard Sandiford writes: >> Richard Biener writes: >>> On Fri, Dec 15, 2017 at 1:48 AM, Richard Sandiford >>> wrote: >>>> Richard Biener writes: >>>>> On Mon, Nov 20, 2017 at 10:02 PM, Richard Sandiford >>>>> wrote: >>>>>> Richard Biener writes: >>>>>>> On Thu, Oct 26, 2017 at 2:06 PM, Richard Biener >>>>>>> wrote: >>>>>>>> On Mon, Oct 23, 2017 at 1:25 PM, Richard Sandiford >>>>>>>> wrote: >>>>>>>>> This patch adds a stub helper routine to provide the mode >>>>>>>>> of a scalar shift amount, given the mode of the values >>>>>>>>> being shifted. >>>>>>>>> >>>>>>>>> One long-standing problem has been to decide what this mode >>>>>>>>> should be for arbitrary rtxes (as opposed to those directly >>>>>>>>> tied to a target pattern). Is it the mode of the shifted >>>>>>>>> elements? Is it word_mode? Or maybe QImode? Is it whatever >>>>>>>>> the corresponding target pattern says? (In which case what >>>>>>>>> should the mode be when the target doesn't have a pattern?) >>>>>>>>> >>>>>>>>> For now the patch picks word_mode, which should be safe on >>>>>>>>> all targets but could perhaps become suboptimal if the helper >>>>>>>>> routine is used more often than it is in this patch. As it >>>>>>>>> stands the patch does not change the generated code. >>>>>>>>> >>>>>>>>> The patch also adds a helper function that constructs rtxes >>>>>>>>> for constant shift amounts, again given the mode of the value >>>>>>>>> being shifted. As well as helping with the SVE patches, this >>>>>>>>> is one step towards allowing CONST_INTs to have a real mode. >>>>>>>> >>>>>>>> I think gen_shift_amount_mode is flawed and while encapsulating >>>>>>>> constant shift amount RTX generation into a gen_int_shift_amount >>>>>>>> looks good to me I'd rather have that ??? in this function (and >>>>>>>> I'd use the mode of the RTX shifted, not word_mode...). >>>>>> >>>>>> OK. I'd gone for word_mode because that's what expand_binop uses >>>>>> for CONST_INTs: >>>>>> >>>>>> op1_mode = (GET_MODE (op1) != VOIDmode >>>>>> ? as_a (GET_MODE (op1)) >>>>>> : word_mode); >>>>>> >>>>>> But using the inner mode should be fine too. The patch below does that. >>>>>> >>>>>>>> In the end it's up to insn recognizing to convert the op to the >>>>>>>> expected mode and for generic RTL it's us that should decide >>>>>>>> on the mode -- on GENERIC the shift amount has to be an >>>>>>>> integer so why not simply use a mode that is large enough to >>>>>>>> make the constant fit? >>>>>> >>>>>> ...but I can do that instead if you think it's better. >>>>>> >>>>>>>> Just throwing in some comments here, RTL isn't my primary >>>>>>>> expertise. >>>>>>> >>>>>>> To add a little bit - shift amounts is maybe the only(?) place >>>>>>> where a modeless CONST_INT makes sense! So "fixing" >>>>>>> that first sounds backwards. >>>>>> >>>>>> But even here they have a mode conceptually, since out-of-range shift >>>>>> amounts are target-defined rather than undefined. E.g. if the target >>>>>> interprets the shift amount as unsigned, then for a shift amount >>>>>> (const_int -1) it matters whether the mode is QImode (and so we're >>>>>> shifting by 255) or HImode (and so we're shifting by 65535. >>>>> >>>>> I think RTL is well-defined (at least I hope so ...) and machine constraints >>>>> need to be modeled explicitely (like embedding an implicit bit_and in >>>>> shift patterns). >>>> >>>> Well, RTL is well-defined in the sense that if you have >>>> >>>> (ashift X (foo:HI ...)) >>>> >>>> then the shift amount must be interpreted as HImode rather than some >>>> other mode. The problem here is to define a default choice of mode for >>>> const_ints, in cases where the shift is being created out of the blue. >>>> >>>> Whether the shift amount is effectively signed or unsigned isn't defined >>>> by RTL without SHIFT_COUNT_TRUNCATED, since the choice only matters for >>>> out-of-range values, and the behaviour for out-of-range RTL shifts is >>>> specifically treated as target-defined without SHIFT_COUNT_TRUNCATED. >>>> >>>> I think the revised patch does implement your suggestion of using the >>>> integer equivalent of the inner mode as the default, but we need to >>>> decide whether to go with it, go with the original word_mode approach >>>> (taken from existing expand_binop code) or something else. Something >>>> else could include the widest supported integer mode, so that we never >>>> change the value. >>> >>> I guess it's pretty arbitrary what we choose (but we might need to adjust >>> targets?). For something like this an appealing choice would be sth >>> that is host and target idependent, like [u]int32_t or given CONST_INT >>> is always 64bits now and signed int64_t aka HOST_WIDE_INT (bad >>> name now). That means it's the "infinite precision" thing that fits >>> into CONST_INT ;) >> >> Sounds OK to me. How about the attached? > > Taking MAX_FIXED_MODE_SIZE into account was bogus, since we'd then just > fail to find a mode. This version has survived the full cross-target > testing. Also bootstrapped & regression-tested on aarch64-linux-gnu, > x86_64-linux-gnu and powerpc64-linux-gnu. OK to install? > > At this stage this is the patch that is holding up the rest of the > approved ones. > > Thanks, > Richard > > > 2017-12-19 Richard Sandiford > Alan Hayward > David Sherwood > > gcc/ > * emit-rtl.h (gen_int_shift_amount): Declare. > * emit-rtl.c (gen_int_shift_amount): New function. > * asan.c (asan_emit_stack_protection): Use gen_int_shift_amount > instead of GEN_INT. > * calls.c (shift_return_value): Likewise. > * cse.c (fold_rtx): Likewise. > * dse.c (find_shift_sequence): Likewise. > * expmed.c (init_expmed_one_mode, store_bit_field_1, expand_shift_1) > (expand_shift, expand_smod_pow2): Likewise. > * lower-subreg.c (shift_cost): Likewise. > * optabs.c (expand_superword_shift, expand_doubleword_mult) > (expand_unop, expand_binop, shift_amt_for_vec_perm_mask) > (expand_vec_perm_var): Likewise. > * simplify-rtx.c (simplify_unary_operation_1): Likewise. > (simplify_binary_operation_1): Likewise. > * combine.c (try_combine, find_split_point, force_int_to_mode) > (simplify_shift_const_1, simplify_shift_const): Likewise. > (change_zero_ext): Likewise. Use simplify_gen_binary. > OK. jeff