From patchwork Fri Dec 6 21:06:25 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kenneth Zadeck X-Patchwork-Id: 298275 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id C830B2C009A for ; Sat, 7 Dec 2013 08:06:44 +1100 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; q=dns; s=default; b=m1dNMl3nrUT3jjNdZ TSvUva4lB3ubLTXPGLKVZWXi6hNLoWhnp23NUVAtTJaMYtzpBglzMfDZXeJjIb31 1eHl3UlzrifkY/MKfciQgQsPa+JMxuTjLWpMxwOQUew/kmkSyFP5EVAJAzl/9Dy/ GGPndsfHO9y1Zf3/SzizoOcVAQ= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; s=default; bh=S2sk8Dfx9MGpo5h/SxT0jwV LkzY=; b=aBxHI9uUCg3oRvmSW56k1lOkmHVpIRTGN2x3bzJ5WTOT/uH1vInYY/S /fU1ishi0pgahlN9VyaE2hCqOFN7rG39Iy8fp7O76WOvjj8bv20VbpC6N9xliL8D Q5fy0HL9aR0XZItnNLLpWzy7bhj2ViH2jDXGRcbHXaDuVKLlbrpY= Received: (qmail 15020 invoked by alias); 6 Dec 2013 21:06:37 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 15000 invoked by uid 89); 6 Dec 2013 21:06:36 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.2 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 X-HELO: mail-yh0-f43.google.com Received: from Unknown (HELO mail-yh0-f43.google.com) (209.85.213.43) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Fri, 06 Dec 2013 21:06:35 +0000 Received: by mail-yh0-f43.google.com with SMTP id a41so918048yho.2 for ; Fri, 06 Dec 2013 13:06:26 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :cc:subject:references:in-reply-to:content-type; bh=UDdDlqcAXCBVeEhJ1zBfavBv3No/PSb36x+GfrkuSzQ=; b=lzWgGpyaDiCFAffNZek9/nsuPD5u0bRoNwSSqL9WfOgqQRNg7V0+esT8UfeJRvZjy1 4boQpRA1OkVp2snkwrM2g4d82fH5877P8/jKPQOuyixJwaPjQH//nuW4VsqFKkcRN9tv Z2Z5D41RIcJk3LHvYTpaOWFy11WfTOnyHBx01lPJNfx1WgE+HzogluDz/TAcEwLYSJIK 7uZbTjg1O0IRzI5RBzVRb5k4W5YKoIyuqlGkSS/TQWhMcn0WH3fJppxDj6yrA6j6lSXE aKzy5Id5fxIIr7Y0KQFNw5FN97BU+a5d9wAN0QaM0O0RDnEnEOEYEqKId2ynmj/f/EE9 qV5A== X-Gm-Message-State: ALoCoQkr0vnqeHAZWeWierm0PFZRzhulMFiWaXqo9q+ksKrtpc9FTAZqw/94FdtItlAngVWc1Lm+ X-Received: by 10.236.15.102 with SMTP id e66mr5199331yhe.69.1386363986684; Fri, 06 Dec 2013 13:06:26 -0800 (PST) Received: from moria.site (pool-98-113-157-168.nycmny.fios.verizon.net. [98.113.157.168]) by mx.google.com with ESMTPSA id h66sm100769263yhb.7.2013.12.06.13.06.25 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Fri, 06 Dec 2013 13:06:25 -0800 (PST) Message-ID: <52A23C51.8040107@naturalbridge.com> Date: Fri, 06 Dec 2013 16:06:25 -0500 From: Kenneth Zadeck User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.1.0 MIME-Version: 1.0 To: Eric Botcazou , Mike Stump CC: gcc-patches@gcc.gnu.org Subject: Re: wide-int, rtl References: <1673383.yVHhgiMuu2@polaris> In-Reply-To: <1673383.yVHhgiMuu2@polaris> On 11/27/2013 11:24 AM, Eric Botcazou wrote: >> Richi has asked the we break the wide-int patch so that the individual port >> and front end maintainers can review their parts without have to go through >> the entire patch. This patch covers the first half of the rtl code. > --- a/gcc/cse.c > +++ b/gcc/cse.c > @@ -2336,15 +2336,23 @@ hash_rtx_cb (const_rtx x, enum machine_mode mode, > + (unsigned int) INTVAL (x)); > return hash; > > + case CONST_WIDE_INT: > + { > + int i; > + for (i = 0; i < CONST_WIDE_INT_NUNITS (x); i++) > + hash += CONST_WIDE_INT_ELT (x, i); > + } > + return hash; > > You can write "for (int i = 0; ..." now and remove the parentheses. > done > --- a/gcc/cselib.c > +++ b/gcc/cselib.c > @@ -1121,15 +1120,23 @@ cselib_hash_rtx (rtx x, int create, enum machine_mode > memmode) > hash += ((unsigned) CONST_INT << 7) + INTVAL (x); > return hash ? hash : (unsigned int) CONST_INT; > > + case CONST_WIDE_INT: > + { > + int i; > + for (i = 0; i < CONST_WIDE_INT_NUNITS (x); i++) > + hash += CONST_WIDE_INT_ELT (x, i); > + } > + return hash; > + > > Likewise. done > --- a/gcc/expmed.c > +++ b/gcc/expmed.c > @@ -3298,23 +3268,13 @@ choose_multiplier (unsigned HOST_WIDE_INT d, int n, > int precision, > pow = n + lgup; > pow2 = n + lgup - precision; > > - /* We could handle this with some effort, but this case is much > - better handled directly with a scc insn, so rely on caller using > - that. */ > - gcc_assert (pow != HOST_BITS_PER_DOUBLE_INT); > > Why removing it? the code no longer has restrictions on the size/mode of the variables. > --- a/gcc/expr.c > +++ b/gcc/expr.c > @@ -722,64 +722,33 @@ convert_modes (enum machine_mode mode, enum machine_mode > oldmode, rtx x, int uns > if (mode == oldmode) > return x; > > - /* There is one case that we must handle specially: If we are converting > - a CONST_INT into a mode whose size is twice HOST_BITS_PER_WIDE_INT and > - we are to interpret the constant as unsigned, gen_lowpart will do > - the wrong if the constant appears negative. What we want to do is > - make the high-order word of the constant zero, not all ones. */ > - > - if (unsignedp && GET_MODE_CLASS (mode) == MODE_INT > - && GET_MODE_BITSIZE (mode) == HOST_BITS_PER_DOUBLE_INT > - && CONST_INT_P (x) && INTVAL (x) < 0) > + if (CONST_SCALAR_INT_P (x) > + && GET_MODE_CLASS (mode) == MODE_INT) > > On a single line. done > > { > - double_int val = double_int::from_uhwi (INTVAL (x)); > - > - /* We need to zero extend VAL. */ > - if (oldmode != VOIDmode) > - val = val.zext (GET_MODE_BITSIZE (oldmode)); > - > - return immed_double_int_const (val, mode); > + /* If the caller did not tell us the old mode, then there is > + not much to do with respect to canonization. We have to assume > + that all the bits are significant. */ > + if (GET_MODE_CLASS (oldmode) != MODE_INT) > + oldmode = MAX_MODE_INT; > + wide_int w = wide_int::from (std::make_pair (x, oldmode), > + GET_MODE_PRECISION (mode), > + unsignedp ? UNSIGNED : SIGNED); > + return immed_wide_int_const (w, mode); > > canonicalization? done > > @@ -5301,10 +5271,10 @@ store_expr (tree exp, rtx target, int call_param_p, > bool nontemporal) > &alt_rtl); > } > > - /* If TEMP is a VOIDmode constant and the mode of the type of EXP is not > - the same as that of TARGET, adjust the constant. This is needed, for > - example, in case it is a CONST_DOUBLE and we want only a word-sized > - value. */ > + /* If TEMP is a VOIDmode constant and the mode of the type of EXP is > + not the same as that of TARGET, adjust the constant. This is > + needed, for example, in case it is a CONST_DOUBLE or > + CONST_WIDE_INT and we want only a word-sized value. */ > if (CONSTANT_P (temp) && GET_MODE (temp) == VOIDmode > && TREE_CODE (exp) != ERROR_MARK > && GET_MODE (target) != TYPE_MODE (TREE_TYPE (exp))) > > Why reformatting the whole comment? it is what emacs did. I have formatted it back. > @@ -9481,11 +9459,19 @@ expand_expr_real_1 (tree exp, rtx target, enum > machine_mode tmode, > return decl_rtl; > > case INTEGER_CST: > - temp = immed_double_const (TREE_INT_CST_LOW (exp), > - TREE_INT_CST_HIGH (exp), mode); > - > - return temp; > - > + { > + tree type = TREE_TYPE (exp); > > Redundant, see the beginning of the function. fixed > + /* One could argue that GET_MODE_PRECISION (TYPE_MODE (type)) > + should always be the same as TYPE_PRECISION (type). > + However, it is not. Since we are converting from tree to > + rtl, we have to expose this ugly truth here. */ > + temp = immed_wide_int_const (wide_int::from > + (exp, > + GET_MODE_PRECISION (TYPE_MODE (type)), > + TYPE_SIGN (type)), > + TYPE_MODE (type)); > + return temp; > + } > > I don't really see how one could argue that, given that there are much fewer > modes than possible type precisions, so please rewrite the comment, e.g.: > > "Given that TYPE_PRECISION (type) is not always equal to > GET_MODE_PRECISION (TYPE_MODE (type)), we need to extend from the former > to the latter according to the signedness of the type". > > What about a fast track where the precisions are indeed equal? > There is not really a faster track here. you still are starting with a tree and converting to an rtx. All that the default one would do would be to access the types precision and sign and use that. > @@ -11145,8 +11131,8 @@ const_vector_from_tree (tree exp) > RTVEC_ELT (v, i) = CONST_FIXED_FROM_FIXED_VALUE (TREE_FIXED_CST (elt), > inner); > else > - RTVEC_ELT (v, i) = immed_double_int_const (tree_to_double_int (elt), > - inner); > + RTVEC_ELT (v, i) > + = immed_wide_int_const (elt, TYPE_MODE (TREE_TYPE (elt))); > } > > return gen_rtx_CONST_VECTOR (mode, v); > > Why replacing inner? fixed. this was just a mistake > > --- a/gcc/machmode.def > +++ b/gcc/machmode.def > @@ -229,6 +229,9 @@ UACCUM_MODE (USA, 4, 16, 16); /* 16.16 */ > UACCUM_MODE (UDA, 8, 32, 32); /* 32.32 */ > UACCUM_MODE (UTA, 16, 64, 64); /* 64.64 */ > > +/* Should be overridden by EXTRA_MODES_FILE if wrong. */ > +#define MAX_BITS_PER_UNIT 8 > + > > What is it for? It's not documented at all. > This requires some discussion as to the direction we want to go. This is put in so that in gen_modes we can compute MAX_BITSIZE_MODE_ANY_INT and MAX_BITSIZE_MODE_ANY_MODE. The problem is that during genmodes we do have access to BITS_PER_UNIT. These two computed symbols are then used as compile time constants in other parts of the compiler to allocate data structures that are guaranteed to be large enough. Richard Sandiford put this in so we would preserve the ability to build a multi-targetted compiler where the targets had different values for BITS_PER_UNIT. So one possibility is that we add some documentation to this effect. A different solution is to notice that there is already code in the compiler that would not support BITS_PER_UNIT being anything but a compile time constant, and so then change the genmodes code so that it produced different symbols that were in terms of units rather than bits and then convert the places in the compiler that use these new symbols by multiplying by BITS_PER_UNIT there. However, that is really only going to work if BITS_PER_UNIT is a compile time constant. do you like one of these or do you have another suggestion? > --- a/gcc/print-rtl.c > +++ b/gcc/print-rtl.c > @@ -617,6 +617,12 @@ print_rtx (const_rtx in_rtx) > fprintf (outfile, " [%s]", s); > } > break; > + > + case CONST_WIDE_INT: > + if (! flag_simple) > + fprintf (outfile, " "); > + cwi_output_hex (outfile, in_rtx); > + break; > #endif > > Remove the if (! flag_simple) test. > done > --- a/gcc/recog.c > +++ b/gcc/recog.c > > /* Returns 1 if OP is an operand that is a constant integer or constant > - floating-point number. */ > + floating-point number of MODE. */ > + > +int > +const_double_operand (rtx op, enum machine_mode mode) > +{ > + return (GET_CODE (op) == CONST_DOUBLE) > + && (GET_MODE (op) == mode || mode == VOIDmode); > +} > > Can CONST_DOUBLEs still represent constant integers? yes. if unless the target has been converted to expect that integers that do not fit into a single hwi are put in CONST_WIDE_INT, they remain in double_ints. > diff --git a/gcc/rtl.def b/gcc/rtl.def > index 15a997b..a76b28b 100644 > --- a/gcc/rtl.def > +++ b/gcc/rtl.def > @@ -345,6 +345,9 @@ DEF_RTL_EXPR(TRAP_IF, "trap_if", "ee", RTX_EXTRA) > /* numeric integer constant */ > DEF_RTL_EXPR(CONST_INT, "const_int", "w", RTX_CONST_OBJ) > > +/* numeric integer constant */ > +DEF_RTL_EXPR(CONST_WIDE_INT, "const_wide_int", "", RTX_CONST_OBJ) > + > > I think that we should consider using a new letter here, for example W. > "" is reserved to very specific cases. > The data structure to hold a CONST_WIDE_INT is actually different than the data structure that holds the rest of the rtx types. a CONST_WIDE_INTs has a single variable length array in it. The length of that array is determined by the size of the constant that it needs to hold. At the suggestion of richi, we made the optimization that their should only be a single one of these arrays allowed. This saved several pointer indirections that would have been required had we made it general enough to hop from variable sized element to variable sized element as you do with the normal rtl scanning. > --- a/gcc/rtl.h > +++ b/gcc/rtl.h > @@ -20,6 +20,7 @@ along with GCC; see the file COPYING3. If not see > #ifndef GCC_RTL_H > #define GCC_RTL_H > > +#include > > Ouch. Can't this really be avoided? > we use the c++ std::make_pair to convert rtls to wide-ints. if we ever add modes to rtl int constants, this would all go away. But there was no reason to reinvent this given that it is part of every c++ implementation. > @@ -336,6 +348,14 @@ struct GTY((chain_next ("RTX_NEXT (&%h)"), > 1 in a VALUE or DEBUG_EXPR is NO_LOC_P in var-tracking.c. */ > unsigned return_val : 1; > > + union { > + /* RTXs are free to use up to 32 bit from here. */ > + > + /* In a CONST_WIDE_INT (aka hwivec_def), this is the number of > + HOST_WIDE_INTs in the hwivec_def. */ > + unsigned GTY ((tag ("CONST_WIDE_INT"))) num_elem:32; > + } GTY ((desc ("GET_CODE (&%0)"))) u2; > + > /* The first element of the operands of this rtx. > The number of operands and their types are controlled > by the `code' field, according to rtl.def. */ > > Why can't this be put into hwivec_def? the comment now describes the reason behind this properly. > > --- a/gcc/simplify-rtx.c > +++ b/gcc/simplify-rtx.c > > + if (op_mode == VOIDmode) > + { > + /* CONST_INT have VOIDmode as the mode. We assume that all > + the bits of the constant are significant, though, this is > + a dangerous assumption as many times CONST_INTs are > + created and used with garbage in the bits outside of the > + precision of the implied mode of the const_int. */ > + op_mode = MAX_MODE_INT; > + } > > Well, if that's really dangerous, we should find another way or at least > stop the compiler before generating wrong code. I think that this may be more about me being honest than anything else. In preparation for this patch we converted a lot of suspect GEN_INTs to gen_int_mode, but there are a lot left. (these were pushed directly to the trunk) The truth is that until we bite the bullet and put modes in rtl integer constants this is always going to be a source of bugs. The good news is that as targets move to supporting wide ints, they will move away from one of the problematic parts of this where the canonical way to test if a const_double has an int inside is to test if the mode is VOIDmode. > > +#if TARGET_SUPPORTS_WIDE_INT == 0 > + /* This assert keeps the simplification from producing a result > + that cannot be represented in a CONST_DOUBLE but a lot of > + upstream callers expect that this function never fails to > + simplify something and so you if you added this to the test > + above the code would die later anyway. If this assert > + happens, you just need to make the port support wide int. */ > + gcc_assert (width <= HOST_BITS_PER_DOUBLE_INT); > +#endif > > Can't we be more forgiving here and in the other similar places? I answered this earlier. but to reiterate, if you need big integers, convert your port to TARGET_SUPPORTS_WIDE_INT, you cannot put a 180 bit constant into 128 bits of data. > > + /* We support any size mode. */ > + max_bitsize = MAX (GET_MODE_BITSIZE (outermode), > + GET_MODE_BITSIZE (innermode)); > > max_bitsize > = MAX (GET_MODE_BITSIZE (outermode), GET_MODE_BITSIZE (innermode)); > > Eric, Since there are a couple of open issues, i am not going ask for ok. But the patch enclosed does do everything that is settled. Kenny Index: gcc/cselib.c =================================================================== --- gcc/cselib.c (revision 205749) +++ gcc/cselib.c (working copy) @@ -1121,11 +1121,8 @@ cselib_hash_rtx (rtx x, int create, enum return hash ? hash : (unsigned int) CONST_INT; case CONST_WIDE_INT: - { - int i; - for (i = 0; i < CONST_WIDE_INT_NUNITS (x); i++) - hash += CONST_WIDE_INT_ELT (x, i); - } + for (i = 0; i < CONST_WIDE_INT_NUNITS (x); i++) + hash += CONST_WIDE_INT_ELT (x, i); return hash; case CONST_DOUBLE: Index: gcc/cse.c =================================================================== --- gcc/cse.c (revision 205749) +++ gcc/cse.c (working copy) @@ -2337,11 +2337,8 @@ hash_rtx_cb (const_rtx x, enum machine_m return hash; case CONST_WIDE_INT: - { - int i; - for (i = 0; i < CONST_WIDE_INT_NUNITS (x); i++) - hash += CONST_WIDE_INT_ELT (x, i); - } + for (i = 0; i < CONST_WIDE_INT_NUNITS (x); i++) + hash += CONST_WIDE_INT_ELT (x, i); return hash; case CONST_DOUBLE: Index: gcc/rtl.h =================================================================== --- gcc/rtl.h (revision 205749) +++ gcc/rtl.h (working copy) @@ -348,7 +348,10 @@ struct GTY((chain_next ("RTX_NEXT (&%h)" unsigned return_val : 1; union { - /* RTXs are free to use up to 32 bit from here. */ + /* The final union field is aligned to 64 bits on LP64 hosts, + giving a 32-bit gap after the fields above. We optimize the + layout for that case and use the gap for extra code-specific + information. */ /* In a CONST_WIDE_INT (aka hwivec_def), this is the number of HOST_WIDE_INTs in the hwivec_def. */ Index: gcc/print-rtl.c =================================================================== --- gcc/print-rtl.c (revision 205749) +++ gcc/print-rtl.c (working copy) @@ -619,8 +619,7 @@ print_rtx (const_rtx in_rtx) break; case CONST_WIDE_INT: - if (! flag_simple) - fprintf (outfile, " "); + fprintf (outfile, " "); cwi_output_hex (outfile, in_rtx); break; #endif Index: gcc/expr.c =================================================================== --- gcc/expr.c (revision 205749) +++ gcc/expr.c (working copy) @@ -727,12 +727,11 @@ convert_modes (enum machine_mode mode, e if (mode == oldmode) return x; - if (CONST_SCALAR_INT_P (x) - && GET_MODE_CLASS (mode) == MODE_INT) + if (CONST_SCALAR_INT_P (x) && GET_MODE_CLASS (mode) == MODE_INT) { - /* If the caller did not tell us the old mode, then there is - not much to do with respect to canonization. We have to assume - that all the bits are significant. */ + /* If the caller did not tell us the old mode, then there is not + much to do with respect to canonicalization. We have to + assume that all the bits are significant. */ if (GET_MODE_CLASS (oldmode) != MODE_INT) oldmode = MAX_MODE_INT; wide_int w = wide_int::from (std::make_pair (x, oldmode), @@ -5295,10 +5294,10 @@ store_expr (tree exp, rtx target, int ca &alt_rtl); } - /* If TEMP is a VOIDmode constant and the mode of the type of EXP is - not the same as that of TARGET, adjust the constant. This is - needed, for example, in case it is a CONST_DOUBLE or - CONST_WIDE_INT and we want only a word-sized value. */ + /* If TEMP is a VOIDmode constant and the mode of the type of EXP is not + the same as that of TARGET, adjust the constant. This is needed, for + example, in case it is a CONST_DOUBLE or CONST_WIDE_INT and we want + only a word-sized value. */ if (CONSTANT_P (temp) && GET_MODE (temp) == VOIDmode && TREE_CODE (exp) != ERROR_MARK && GET_MODE (target) != TYPE_MODE (TREE_TYPE (exp))) @@ -9477,19 +9476,18 @@ expand_expr_real_1 (tree exp, rtx target return decl_rtl; case INTEGER_CST: - { - tree type = TREE_TYPE (exp); - /* One could argue that GET_MODE_PRECISION (TYPE_MODE (type)) - should always be the same as TYPE_PRECISION (type). - However, it is not. Since we are converting from tree to - rtl, we have to expose this ugly truth here. */ - temp = immed_wide_int_const (wide_int::from - (exp, - GET_MODE_PRECISION (TYPE_MODE (type)), - TYPE_SIGN (type)), - TYPE_MODE (type)); - return temp; - } + /* "Given that TYPE_PRECISION (type) is not always equal to + GET_MODE_PRECISION (TYPE_MODE (type)), we need to extend from + the former to the latter according to the signedness of the + type". */ + + temp = immed_wide_int_const (wide_int::from + (exp, + GET_MODE_PRECISION (TYPE_MODE (type)), + TYPE_SIGN (type)), + TYPE_MODE (type)); + return temp; + case VECTOR_CST: { tree tmp = NULL_TREE; @@ -11149,8 +11147,7 @@ const_vector_from_tree (tree exp) RTVEC_ELT (v, i) = CONST_FIXED_FROM_FIXED_VALUE (TREE_FIXED_CST (elt), inner); else - RTVEC_ELT (v, i) - = immed_wide_int_const (elt, TYPE_MODE (TREE_TYPE (elt))); + RTVEC_ELT (v, i) = immed_wide_int_const (elt, inner); } return gen_rtx_CONST_VECTOR (mode, v);