From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19337 invoked by alias); 15 Nov 2018 08:29:40 -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 19323 invoked by uid 89); 15 Nov 2018 08:29:39 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-6.1 required=5.0 tests=BAYES_00,GIT_PATCH_2,KAM_ASCII_DIVIDERS,SPF_PASS autolearn=ham version=3.3.2 spammy=HTo:U*pinskia X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 15 Nov 2018 08:29:36 +0000 Received: from relay1.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id B9739AA71; Thu, 15 Nov 2018 08:29:33 +0000 (UTC) Date: Thu, 15 Nov 2018 08:29:00 -0000 From: Richard Biener To: Andrew Pinski cc: GCC Patches Subject: Re: [PATCH][RFC] Introduce BIT_FIELD_INSERT In-Reply-To: Message-ID: References: User-Agent: Alpine 2.20 (LSU 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-SW-Source: 2018-11/txt/msg01347.txt.bz2 On Wed, 14 Nov 2018, Andrew Pinski wrote: > On Fri, May 13, 2016 at 3:51 AM Richard Biener wrote: > > > > > > The following patch adds BIT_FIELD_INSERT, an operation to > > facilitate doing bitfield inserts on registers (as opposed > > to currently where we'd have a BIT_FIELD_REF store). > > > > Originally this was developed as part of bitfield lowering > > where bitfield stores were lowered into read-modify-write > > cycles and the modify part, instead of doing shifting and masking, > > be kept in a more high-level form to ease combining them. > > > > A second use case (the above is still valid) is vector element > > inserts which we currently can only do via memory or > > by extracting all components and re-building the vector using > > a CONSTRUCTOR. For this second use case I added code > > re-writing the BIT_FIELD_REF stores the C family FEs produce > > into BIT_FIELD_INSERT when update-address-taken can otherwise > > re-write a decl into SSA form (the testcase shows we miss > > a similar opportunity with the MEM_REF form of a vector insert, > > I plan to fix that for the final submission). > > > > One speciality of BIT_FIELD_INSERT as opposed to BIT_FIELD_REF > > is that the size of the insertion is given implicitely via the > > type size/precision of the value to insert. That avoids > > introducing ways to have quaternary ops in folding and GIMPLE stmts. > > > > Bootstrapped and tested on x86_64-unknown-linux-gnu. > > > > Richard. > > > > 2011-06-16 Richard Guenther > > > > PR tree-optimization/29756 > > * tree.def (BIT_FIELD_INSERT): New tcc_expression tree code. > > * expr.c (expand_expr_real_2): Handle BIT_FIELD_INSERT. > > * fold-const.c (operand_equal_p): Likewise. > > (fold_ternary_loc): Add constant folding of BIT_FIELD_INSERT. > > * gimplify.c (gimplify_expr): Handle BIT_FIELD_INSERT. > > * tree-inline.c (estimate_operator_cost): Likewise. > > * tree-pretty-print.c (dump_generic_node): Likewise. > > * tree-ssa-operands.c (get_expr_operands): Likewise. > > * cfgexpand.c (expand_debug_expr): Likewise. > > * gimple-pretty-print.c (dump_ternary_rhs): Likewise. > > * gimple.c (get_gimple_rhs_num_ops): Handle BIT_FIELD_INSERT. > > * tree-cfg.c (verify_gimple_assign_ternary): Verify BIT_FIELD_INSERT. > > > > * tree-ssa.c (non_rewritable_lvalue_p): We can rewrite > > vector inserts using BIT_FIELD_REF on the lhs. > > (execute_update_addresses_taken): Do it. > > > > * gcc.dg/tree-ssa/vector-6.c: New testcase. > > > > Index: trunk/gcc/expr.c > > =================================================================== > > *** trunk.orig/gcc/expr.c 2016-05-12 13:40:30.704262951 +0200 > > --- trunk/gcc/expr.c 2016-05-12 15:40:32.481225744 +0200 > > *************** expand_expr_real_2 (sepops ops, rtx targ > > *** 9358,9363 **** > > --- 9358,9380 ---- > > target = expand_vec_cond_expr (type, treeop0, treeop1, treeop2, target); > > return target; > > > > + case BIT_FIELD_INSERT: > > + { > > + unsigned bitpos = tree_to_uhwi (treeop2); > > + unsigned bitsize; > > + if (INTEGRAL_TYPE_P (TREE_TYPE (treeop1))) > > + bitsize = TYPE_PRECISION (TREE_TYPE (treeop1)); > > + else > > + bitsize = tree_to_uhwi (TYPE_SIZE (TREE_TYPE (treeop1))); > > + rtx op0 = expand_normal (treeop0); > > + rtx op1 = expand_normal (treeop1); > > + rtx dst = gen_reg_rtx (mode); > > + emit_move_insn (dst, op0); > > + store_bit_field (dst, bitsize, bitpos, 0, 0, > > + TYPE_MODE (TREE_TYPE (treeop1)), op1, false); > > + return dst; > > + } > > + > > default: > > gcc_unreachable (); > > } > > Index: trunk/gcc/fold-const.c > > =================================================================== > > *** trunk.orig/gcc/fold-const.c 2016-05-12 13:40:30.704262951 +0200 > > --- trunk/gcc/fold-const.c 2016-05-13 09:41:13.509812127 +0200 > > *************** operand_equal_p (const_tree arg0, const_ > > *** 3163,3168 **** > > --- 3163,3169 ---- > > > > case VEC_COND_EXPR: > > case DOT_PROD_EXPR: > > + case BIT_FIELD_INSERT: > > return OP_SAME (0) && OP_SAME (1) && OP_SAME (2); > > > > default: > > *************** fold_ternary_loc (location_t loc, enum t > > *** 11870,11875 **** > > --- 11871,11916 ---- > > } > > return NULL_TREE; > > > > + case BIT_FIELD_INSERT: > > + /* Perform (partial) constant folding of BIT_FIELD_INSERT. */ > > + if (TREE_CODE (arg0) == INTEGER_CST > > + && TREE_CODE (arg1) == INTEGER_CST) > > + { > > + unsigned HOST_WIDE_INT bitpos = tree_to_uhwi (op2); > > + unsigned bitsize = TYPE_PRECISION (TREE_TYPE (arg1)); > > + wide_int tem = wi::bit_and (arg0, > > + wi::shifted_mask (bitpos, bitsize, true, > > + TYPE_PRECISION (type))); > > + wide_int tem2 > > + = wi::lshift (wi::zext (wi::to_wide (arg1, TYPE_PRECISION (type)), > > + bitsize), bitpos); > > + return wide_int_to_tree (type, wi::bit_or (tem, tem2)); > > + } > > This seems incorrect for the case where BYTES_BIG_ENDIAN as far as I > can tell. With BYTES_BIG_ENDIAN, the bits position starts most > significiant rather than the least significiant. You mean the bitpos operand of BIT_FIELD_INSERT works in a different way? I see the BIT_FIELD_REF folding uses native_encode/interpret but only handles byte-aligned references. I suppose the BIT_INSERT_EXPR case (you are following up an old patch) could do the same. > Sorry I am bring > this up after this has been in the tree for a long time but I finally > got around to testing my bit-field lowering on a few big-endian > targets and ran into this issue. So - can you fix it please? Also note that the VECTOR_CST case (as in BIT_FIELD_REF) seems to be inconsistent here and counts "bits" in a different way? Thanks, Richard. > Thanks, > Andrew Pinski > > > + else if (TREE_CODE (arg0) == VECTOR_CST > > + && CONSTANT_CLASS_P (arg1) > > + && types_compatible_p (TREE_TYPE (TREE_TYPE (arg0)), > > + TREE_TYPE (arg1))) > > + { > > + unsigned HOST_WIDE_INT bitpos = tree_to_uhwi (op2); > > + unsigned HOST_WIDE_INT elsize > > + = tree_to_uhwi (TYPE_SIZE (TREE_TYPE (arg1))); > > + if (bitpos % elsize == 0) > > + { > > + unsigned k = bitpos / elsize; > > + if (operand_equal_p (VECTOR_CST_ELT (arg0, k), arg1, 0)) > > + return arg0; > > + else > > + { > > + tree *elts = XALLOCAVEC (tree, TYPE_VECTOR_SUBPARTS (type)); > > + memcpy (elts, VECTOR_CST_ELTS (arg0), > > + sizeof (tree) * TYPE_VECTOR_SUBPARTS (type)); > > + elts[k] = arg1; > > + return build_vector (type, elts); > > + } > > + } > > + } > > + return NULL_TREE; > > + > > default: > > return NULL_TREE; > > } /* switch (code) */ > > Index: trunk/gcc/gimplify.c > > =================================================================== > > *** trunk.orig/gcc/gimplify.c 2016-05-12 13:40:30.704262951 +0200 > > --- trunk/gcc/gimplify.c 2016-05-12 13:56:18.679120641 +0200 > > *************** gimplify_expr (tree *expr_p, gimple_seq > > *** 10936,10941 **** > > --- 10936,10945 ---- > > /* Classified as tcc_expression. */ > > goto expr_3; > > > > + case BIT_FIELD_INSERT: > > + /* Argument 3 is a constant. */ > > + goto expr_2; > > + > > case POINTER_PLUS_EXPR: > > { > > enum gimplify_status r0, r1; > > Index: trunk/gcc/tree-inline.c > > =================================================================== > > *** trunk.orig/gcc/tree-inline.c 2016-05-12 13:40:30.704262951 +0200 > > --- trunk/gcc/tree-inline.c 2016-05-12 13:42:45.465811959 +0200 > > *************** estimate_operator_cost (enum tree_code c > > *** 3941,3946 **** > > --- 3941,3950 ---- > > return weights->div_mod_cost; > > return 1; > > > > + /* Bit-field insertion needs several shift and mask operations. */ > > + case BIT_FIELD_INSERT: > > + return 3; > > + > > default: > > /* We expect a copy assignment with no operator. */ > > gcc_assert (get_gimple_rhs_class (code) == GIMPLE_SINGLE_RHS); > > Index: trunk/gcc/tree-pretty-print.c > > =================================================================== > > *** trunk.orig/gcc/tree-pretty-print.c 2016-05-12 13:40:30.704262951 +0200 > > --- trunk/gcc/tree-pretty-print.c 2016-05-12 14:30:05.781944740 +0200 > > *************** dump_generic_node (pretty_printer *pp, t > > *** 1876,1881 **** > > --- 1876,1898 ---- > > pp_greater (pp); > > break; > > > > + case BIT_FIELD_INSERT: > > + pp_string (pp, "BIT_FIELD_INSERT <"); > > + dump_generic_node (pp, TREE_OPERAND (node, 0), spc, flags, false); > > + pp_string (pp, ", "); > > + dump_generic_node (pp, TREE_OPERAND (node, 1), spc, flags, false); > > + pp_string (pp, ", "); > > + dump_generic_node (pp, TREE_OPERAND (node, 2), spc, flags, false); > > + pp_string (pp, " ("); > > + if (INTEGRAL_TYPE_P (TREE_TYPE (TREE_OPERAND (node, 1)))) > > + pp_decimal_int (pp, > > + TYPE_PRECISION (TREE_TYPE (TREE_OPERAND (node, 1)))); > > + else > > + dump_generic_node (pp, TYPE_SIZE (TREE_TYPE (TREE_OPERAND (node, 1))), > > + spc, flags, false); > > + pp_string (pp, " bits)>"); > > + break; > > + > > case ARRAY_REF: > > case ARRAY_RANGE_REF: > > op0 = TREE_OPERAND (node, 0); > > Index: trunk/gcc/tree-ssa-operands.c > > =================================================================== > > *** trunk.orig/gcc/tree-ssa-operands.c 2016-05-12 13:42:45.465811959 +0200 > > --- trunk/gcc/tree-ssa-operands.c 2016-05-12 13:48:26.881736503 +0200 > > *************** get_expr_operands (struct function *fn, > > *** 833,838 **** > > --- 833,839 ---- > > get_expr_operands (fn, stmt, &TREE_OPERAND (expr, 0), flags); > > return; > > > > + case BIT_FIELD_INSERT: > > case COMPOUND_EXPR: > > case OBJ_TYPE_REF: > > case ASSERT_EXPR: > > Index: trunk/gcc/tree.def > > =================================================================== > > *** trunk.orig/gcc/tree.def 2016-05-12 13:40:30.704262951 +0200 > > --- trunk/gcc/tree.def 2016-05-12 13:47:09.972852423 +0200 > > *************** DEFTREECODE (ADDR_EXPR, "addr_expr", tcc > > *** 852,857 **** > > --- 852,868 ---- > > descriptor of type ptr_mode. */ > > DEFTREECODE (FDESC_EXPR, "fdesc_expr", tcc_expression, 2) > > > > + /* Given a word, a value and a bitfield position within the word, > > + produce the value that results if replacing the > > + described parts of word with value. > > + Operand 0 is a tree for the word of integral type; > > + Operand 1 is a tree for the value of integral type; > > + Operand 2 is a tree giving the constant position of the first referenced bit; > > + The number of bits replaced is given by the precision of the value > > + type if that is integral or by its size if it is non-integral. > > + The replaced bits shall be fully inside the word. */ > > + DEFTREECODE (BIT_FIELD_INSERT, "bit_field_insert", tcc_expression, 3) > > + > > /* Given two real or integer operands of the same type, > > returns a complex value of the corresponding complex type. */ > > DEFTREECODE (COMPLEX_EXPR, "complex_expr", tcc_binary, 2) > > Index: trunk/gcc/cfgexpand.c > > =================================================================== > > *** trunk.orig/gcc/cfgexpand.c 2016-05-12 13:42:45.469812005 +0200 > > --- trunk/gcc/cfgexpand.c 2016-05-13 11:48:04.513407495 +0200 > > *************** expand_debug_expr (tree exp) > > *** 5025,5030 **** > > --- 5025,5031 ---- > > case FIXED_CONVERT_EXPR: > > case OBJ_TYPE_REF: > > case WITH_SIZE_EXPR: > > + case BIT_FIELD_INSERT: > > return NULL; > > > > case DOT_PROD_EXPR: > > Index: trunk/gcc/gimple-pretty-print.c > > =================================================================== > > *** trunk.orig/gcc/gimple-pretty-print.c 2016-05-12 11:23:09.261375157 +0200 > > --- trunk/gcc/gimple-pretty-print.c 2016-05-12 14:57:22.096175579 +0200 > > *************** dump_ternary_rhs (pretty_printer *buffer > > *** 479,484 **** > > --- 479,502 ---- > > pp_greater (buffer); > > break; > > > > + case BIT_FIELD_INSERT: > > + pp_string (buffer, "BIT_FIELD_INSERT <"); > > + dump_generic_node (buffer, gimple_assign_rhs1 (gs), spc, flags, false); > > + pp_string (buffer, ", "); > > + dump_generic_node (buffer, gimple_assign_rhs2 (gs), spc, flags, false); > > + pp_string (buffer, ", "); > > + dump_generic_node (buffer, gimple_assign_rhs3 (gs), spc, flags, false); > > + pp_string (buffer, " ("); > > + if (INTEGRAL_TYPE_P (TREE_TYPE (gimple_assign_rhs2 (gs)))) > > + pp_decimal_int (buffer, > > + TYPE_PRECISION (TREE_TYPE (gimple_assign_rhs2 (gs)))); > > + else > > + dump_generic_node (buffer, > > + TYPE_SIZE (TREE_TYPE (gimple_assign_rhs2 (gs))), > > + spc, flags, false); > > + pp_string (buffer, " bits)>"); > > + break; > > + > > default: > > gcc_unreachable (); > > } > > Index: trunk/gcc/gimple.c > > =================================================================== > > *** trunk.orig/gcc/gimple.c 2016-05-12 13:40:30.704262951 +0200 > > --- trunk/gcc/gimple.c 2016-05-12 14:49:37.066994969 +0200 > > *************** get_gimple_rhs_num_ops (enum tree_code c > > *** 2044,2049 **** > > --- 2044,2050 ---- > > || (SYM) == REALIGN_LOAD_EXPR \ > > || (SYM) == VEC_COND_EXPR \ > > || (SYM) == VEC_PERM_EXPR \ > > + || (SYM) == BIT_FIELD_INSERT \ > > || (SYM) == FMA_EXPR) ? GIMPLE_TERNARY_RHS \ > > : ((SYM) == CONSTRUCTOR \ > > || (SYM) == OBJ_TYPE_REF \ > > Index: trunk/gcc/tree-cfg.c > > =================================================================== > > *** trunk.orig/gcc/tree-cfg.c 2016-05-06 14:38:33.959495081 +0200 > > --- trunk/gcc/tree-cfg.c 2016-05-13 09:25:01.670630730 +0200 > > *************** verify_gimple_assign_ternary (gassign *s > > *** 4155,4160 **** > > --- 4155,4207 ---- > > > > return false; > > > > + case BIT_FIELD_INSERT: > > + if (! useless_type_conversion_p (lhs_type, rhs1_type)) > > + { > > + error ("type mismatch in BIT_FIELD_INSERT"); > > + debug_generic_expr (lhs_type); > > + debug_generic_expr (rhs1_type); > > + return true; > > + } > > + if (! ((INTEGRAL_TYPE_P (rhs1_type) > > + && INTEGRAL_TYPE_P (rhs2_type)) > > + || (VECTOR_TYPE_P (rhs1_type) > > + && types_compatible_p (TREE_TYPE (rhs1_type), rhs2_type)))) > > + { > > + error ("not allowed type combination in BIT_FIELD_INSERT"); > > + debug_generic_expr (rhs1_type); > > + debug_generic_expr (rhs2_type); > > + return true; > > + } > > + if (! tree_fits_uhwi_p (rhs3) > > + || ! tree_fits_uhwi_p (TYPE_SIZE (rhs2_type))) > > + { > > + error ("invalid position or size in BIT_FIELD_INSERT"); > > + return true; > > + } > > + if (INTEGRAL_TYPE_P (rhs1_type)) > > + { > > + unsigned HOST_WIDE_INT bitpos = tree_to_uhwi (rhs3); > > + if (bitpos >= TYPE_PRECISION (rhs1_type) > > + || (bitpos + TYPE_PRECISION (rhs2_type) > > + > TYPE_PRECISION (rhs1_type))) > > + { > > + error ("insertion out of range in BIT_FIELD_INSERT"); > > + return true; > > + } > > + } > > + else if (VECTOR_TYPE_P (rhs1_type)) > > + { > > + unsigned HOST_WIDE_INT bitpos = tree_to_uhwi (rhs3); > > + unsigned HOST_WIDE_INT bitsize = tree_to_uhwi (TYPE_SIZE (rhs2_type)); > > + if (bitpos % bitsize != 0) > > + { > > + error ("vector insertion not at element boundary"); > > + return true; > > + } > > + } > > + return false; > > + > > case DOT_PROD_EXPR: > > case REALIGN_LOAD_EXPR: > > /* FIXME. */ > > Index: trunk/gcc/tree-ssa.c > > =================================================================== > > *** trunk.orig/gcc/tree-ssa.c 2016-05-13 09:38:02.263611726 +0200 > > --- trunk/gcc/tree-ssa.c 2016-05-13 09:50:31.020226585 +0200 > > *************** non_rewritable_lvalue_p (tree lhs) > > *** 1318,1323 **** > > --- 1318,1335 ---- > > return false; > > } > > > > + /* A vector-insert using a BIT_FIELD_REF is rewritable using > > + BIT_FIELD_INSERT. */ > > + if (TREE_CODE (lhs) == BIT_FIELD_REF > > + && DECL_P (TREE_OPERAND (lhs, 0)) > > + && VECTOR_TYPE_P (TREE_TYPE (TREE_OPERAND (lhs, 0))) > > + /* && bitsize % element-size == 0 */ > > + && types_compatible_p (TREE_TYPE (lhs), > > + TREE_TYPE (TREE_TYPE (TREE_OPERAND (lhs, 0)))) > > + && (tree_to_uhwi (TREE_OPERAND (lhs, 2)) > > + % tree_to_uhwi (TYPE_SIZE (TREE_TYPE (lhs)))) == 0) > > + return false; > > + > > return true; > > } > > > > *************** execute_update_addresses_taken (void) > > *** 1536,1541 **** > > --- 1548,1576 ---- > > stmt = gsi_stmt (gsi); > > unlink_stmt_vdef (stmt); > > update_stmt (stmt); > > + continue; > > + } > > + > > + /* Rewrite a vector insert via a BIT_FIELD_REF on the LHS > > + into a BIT_FIELD_INSERT. */ > > + if (TREE_CODE (lhs) == BIT_FIELD_REF > > + && DECL_P (TREE_OPERAND (lhs, 0)) > > + && VECTOR_TYPE_P (TREE_TYPE (TREE_OPERAND (lhs, 0))) > > + && types_compatible_p (TREE_TYPE (lhs), > > + TREE_TYPE (TREE_TYPE > > + (TREE_OPERAND (lhs, 0)))) > > + && (tree_to_uhwi (TREE_OPERAND (lhs, 2)) > > + % tree_to_uhwi (TYPE_SIZE (TREE_TYPE (lhs))) == 0)) > > + { > > + tree var = TREE_OPERAND (lhs, 0); > > + tree val = gimple_assign_rhs1 (stmt); > > + tree bitpos = TREE_OPERAND (lhs, 2); > > + gimple_assign_set_lhs (stmt, var); > > + gimple_assign_set_rhs_with_ops > > + (&gsi, BIT_FIELD_INSERT, var, val, bitpos); > > + stmt = gsi_stmt (gsi); > > + unlink_stmt_vdef (stmt); > > + update_stmt (stmt); > > continue; > > } > > > > Index: trunk/gcc/testsuite/gcc.dg/tree-ssa/vector-6.c > > =================================================================== > > *** /dev/null 1970-01-01 00:00:00.000000000 +0000 > > --- trunk/gcc/testsuite/gcc.dg/tree-ssa/vector-6.c 2016-05-13 09:54:16.026814995 +0200 > > *************** > > *** 0 **** > > --- 1,34 ---- > > + /* { dg-do compile } */ > > + /* { dg-options "-O -fdump-tree-ccp1" } */ > > + > > + typedef int v4si __attribute__((vector_size (4 * sizeof (int)))); > > + > > + v4si test1 (v4si v, int i) > > + { > > + ((int *)&v)[0] = i; > > + return v; > > + } > > + > > + v4si test2 (v4si v, int i) > > + { > > + int *p = (int *)&v; > > + *p = i; > > + return v; > > + } > > + > > + v4si test3 (v4si v, int i) > > + { > > + ((int *)&v)[3] = i; > > + return v; > > + } > > + > > + v4si test4 (v4si v, int i) > > + { > > + int *p = (int *)&v; > > + p += 3; > > + *p = i; > > + return v; > > + } > > + > > + /* { dg-final { scan-tree-dump-times "Now a gimple register: v" 2 "ccp1" } } */ > > + /* { dg-final { scan-tree-dump-times "Now a gimple register: v" 4 "ccp1" { xfail *-*-* } } } */ > > -- Richard Biener SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)