From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20625 invoked by alias); 22 Jun 2004 06:36:32 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 20617 invoked from network); 22 Jun 2004 06:36:30 -0000 Received: from unknown (HELO mx2.redhat.com) (66.187.237.31) by sourceware.org with SMTP; 22 Jun 2004 06:36:30 -0000 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.12.10/8.12.10) with ESMTP id i5M6XYp4025902; Tue, 22 Jun 2004 02:33:34 -0400 Received: from potter.sfbay.redhat.com (potter.sfbay.redhat.com [172.16.27.15]) by int-mx2.corp.redhat.com (8.11.6/8.11.6) with ESMTP id i5M6aTw30513; Tue, 22 Jun 2004 02:36:29 -0400 Received: from frothingslosh.sfbay.redhat.com (frothingslosh.sfbay.redhat.com [172.16.24.27]) by potter.sfbay.redhat.com (8.11.6/8.11.6) with ESMTP id i5M6aTP19611; Mon, 21 Jun 2004 23:36:29 -0700 Received: from frothingslosh.sfbay.redhat.com (localhost.localdomain [127.0.0.1]) by frothingslosh.sfbay.redhat.com (8.12.10/8.12.10) with ESMTP id i5M6aTQw002790; Mon, 21 Jun 2004 23:36:29 -0700 Received: (from rth@localhost) by frothingslosh.sfbay.redhat.com (8.12.10/8.12.10/Submit) id i5M6aTnF002788; Mon, 21 Jun 2004 23:36:29 -0700 X-Authentication-Warning: frothingslosh.sfbay.redhat.com: rth set sender to rth@redhat.com using -f Date: Tue, 22 Jun 2004 11:00:00 -0000 From: Richard Henderson To: Richard Kenner Cc: gcc-patches@gcc.gnu.org Subject: Re: Patch to allow Ada to work with tree-ssa Message-ID: <20040622063629.GA2646@redhat.com> Mail-Followup-To: Richard Henderson , Richard Kenner , gcc-patches@gcc.gnu.org References: <10406220317.AA03680@vlsi1.ultra.nyu.edu> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <10406220317.AA03680@vlsi1.ultra.nyu.edu> User-Agent: Mutt/1.4.1i X-SW-Source: 2004-06/txt/msg01729.txt.bz2 On Mon, Jun 21, 2004 at 11:17:27PM -0400, Richard Kenner wrote: > *************** gimplify_expr_stmt (tree *stmt_p) > *** 244,248 **** > > if (stmt == NULL_TREE) > - stmt = build_empty_stmt (); > + stmt = alloc_stmt_list (); Huh? > ! gimplify_stmt (&alloc_stmt); > ! append_to_statement_list(alloc_stmt, stmt_p); gimplify_and_add. > --- 148,151 ---- > *** c-typeck.c 21 Jun 2004 09:15:20 -0000 1.323 > --- c-typeck.c 21 Jun 2004 22:49:52 -0000 > *************** default_function_array_conversion (tree > *** 1227,1234 **** > if (TREE_CODE (exp) == VAR_DECL) > { > ! /* ??? This is not really quite correct > ! in that the type of the operand of ADDR_EXPR > ! is not the target type of the type of the ADDR_EXPR itself. > ! Question is, can this lossage be avoided? */ > adr = build1 (ADDR_EXPR, ptrtype, exp); > if (!c_mark_addressable (exp)) > --- 1227,1234 ---- > if (TREE_CODE (exp) == VAR_DECL) > { > ! /* We are making an ADDR_EXPR of ptrtype. This is a valid > ! ADDR_EXPR because it's the best way of representing what > ! happens in C when we take the address of an array and place > ! it in a pointer to the element type. */ Like I said, probably &array[0] would be a better choice. > *************** get_inner_reference (tree exp, HOST_WIDE > *** 5677,5680 **** > --- 5669,5736 ---- > } > > + /* Return a tree of sizetype representing the size, in bytes, of the element > + of EXP, an ARRAY_REF. */ > + > + tree > + array_ref_element_size (tree exp) I wonder if "stride" might be a better term than "size"? > + /* If a size was specified in the ARRAY_REF, it's the size measured > + in alignment units of the element type. So multiply by that value. */ > + if (aligned_size) > + return size_binop (MULT_EXPR, aligned_size, > + size_int (TYPE_ALIGN (elmt_type) / BITS_PER_UNIT)); Is that multiplication really healthy? Given that we've done all of the heavy lifting optimization that we're going to do, I'd think we'd get a division and a multiplication, and not be able to merge them. Anyway, what are you hoping to gain with not exposing the multiplication to the tree optimizers? > + /* Otherwise, return a zero of the appropriate type. */ > + return fold_convert (TREE_TYPE (TREE_OPERAND (exp, 1)), integer_zero_node); Are you assuming that we've canonicalized the index operand to the domain type? I was not aware that was happening at present... > + /* Forward declarations. */ > + static hashval_t gimple_tree_hash (const void *); Please don't add unnecessary forward declarations. > + /* Unshare all the trees in BODY_P, a pointer to the body of FNDECL, and the > + bodies of any nested functions. */ Why would you need to do this? What's getting shared between outer and inner functions? If it's related to TYPE_SIZE_UNIT and friends, it seems easier to me to just always copy those expressions when we pull them out. Indeed, that's what I was already doing for the places that we did handle variable sized types before. > /* If a NOP conversion is changing a pointer to array of foo to a pointer > ! to foo, embed that change in the ADDR_EXPR by converting > ! T array[U]; > ! (T *)&array > ==> > ! &array[L] > ! where L is the lower bound. Only do this for constant lower bound since > ! we have no place to put any statements made during gimplification of > ! the lower bound. */ Well, we certainly *can* give gimplify_conversion the pre-queue from gimplify_expr, but it seems unlikely that we'll necessarily see that non-constant lower-bound and generate (symbol_ref array). I.e. avoid all arithmetic when we get to the rtl level. > + /* The lower bound and element sizes must be constant. */ > + if (TREE_CODE (TYPE_SIZE_UNIT (dctype)) != INTEGER_CST > + || !TYPE_DOMAIN (datype) || !TYPE_MIN_VALUE (TYPE_DOMAIN (datype)) Doesn't null domain imply zero lower bound? > + build_addr_expr_with_type (tree t, tree ptrtype) ... > + build_addr_expr (tree t) What's wrong with the build_fold functions in fold-const.c? > ! /* We can either handle one REALPART_EXPR or IMAGEPART_EXPR or > ! nest of handled components. */ > ! if (TREE_CODE (*expr_p) == REALPART_EXPR > ! || TREE_CODE (*expr_p) == IMAGPART_EXPR) > ! p = &TREE_OPERAND (*expr_p, 0); > ! else > ! for (p = expr_p; handled_component_p (*p); p = &TREE_OPERAND (*p, 0)) > VARRAY_PUSH_TREE (stack, *p); I see no reason to separate out the complex accessors here. Why? > *************** gimplify_addr_expr (tree *expr_p, tree * > ! case VIEW_CONVERT_EXPR: > ! /* Take the address of our operand and then convert it to the type of > ! this ADDR_EXPR. */ > ! *expr_p = fold_convert (TREE_TYPE (expr), > ! build_addr_expr (TREE_OPERAND (op0, 0))); What are you doing to prevent this from breaking aliasing? This looks just like "(int *)&some_float" to me... I guess you just hope that the source and destination type alias sets are compatible? Perhaps we should be validating? > ! /* This is probably a _REF that contains something nested that > ! has side effects. Recurse through the operands to find it. */ > ! enum tree_code code = TREE_CODE (*expr_p); > ! > ! if (code == COMPONENT_REF > ! || code == REALPART_EXPR || code == IMAGPART_EXPR) > ! gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, post_p, > ! gimple_test_f, fallback); > ! else if (code == ARRAY_REF || code == ARRAY_RANGE_REF) > ! { > ! gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, post_p, > ! gimple_test_f, fallback); > ! gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p, post_p, > ! gimple_test_f, fallback); I don't understand what you're doing here. In what way could you have not already gimplified these references? > + tree > + gimplify_type_sizes (tree type) This would seem a bit better with a list_p argument. > + gimplify_expr (expr_p, &pre, &post, is_gimple_val, fb_rvalue); > + > + if (pre) > + append_to_statement_list (pre, stmt_p); > + if (post) > + append_to_statement_list (post, stmt_p); It is incorrect to give gimplify_expr a post-queue here, because *expr_p is going to be used after *stmt_p. Either gimplify_expr will find a workaround for the missing post-queue, or we'll abort. As it is we'll generate incorrect code. > *************** get_base_address (tree t) > ! else if (handled_component_p (t)) > ! t = get_base_address (TREE_OPERAND (t, 0)); Loop + recursion? Pick one. r~