From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 45345 invoked by alias); 24 Apr 2015 04:23:02 -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 45333 invoked by uid 89); 24 Apr 2015 04:23:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.9 required=5.0 tests=AWL,BAYES_05,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 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 (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 24 Apr 2015 04:22:59 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 7E9D6A10C8; Fri, 24 Apr 2015 04:22:57 +0000 (UTC) Received: from [10.10.116.43] ([10.10.116.43]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t3O4MuW3003359; Fri, 24 Apr 2015 00:22:56 -0400 Message-ID: <5539C519.8070004@redhat.com> Date: Fri, 24 Apr 2015 04:23:00 -0000 From: Jason Merrill User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Kai Tietz CC: gcc-patches List Subject: C++ delayed folding branch review Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2015-04/txt/msg01484.txt.bz2 > + expr = fold (expr); > /* This may happen, because for LHS op= RHS we preevaluate > RHS and create C_MAYBE_CONST_EXPR >, which > means we could no longer see the code of the EXPR. */ > if (TREE_CODE (expr) == C_MAYBE_CONST_EXPR) > expr = C_MAYBE_CONST_EXPR_EXPR (expr); > if (TREE_CODE (expr) == SAVE_EXPR) > - expr = TREE_OPERAND (expr, 0); > + expr = fold (TREE_OPERAND (expr, 0)); How about moving the first fold after the SAVE_EXPR block, so that we only need to call fold once? > + case NEGATE_EXPR: > + case BIT_NOT_EXPR: > + case CONVERT_EXPR: > + case VIEW_CONVERT_EXPR: > + case NOP_EXPR: > + case FIX_TRUNC_EXPR: > + { > + tree op1 = TREE_OPERAND (expr, 0); > + tree fop1 = fold (op1); > + if (fop1 && op1 != fop1) > + fop1 = fold_build1_loc (loc, TREE_CODE (expr), TREE_TYPE (expr), > + fop1); Isn't this redundant with the call to fold above? If not, it seems that the above call should be to *_fully_fold. I suppose we want an entry point defined by both front ends that c-common code can call which does full folding of an expression. > @@ -597,9 +597,9 @@ null_member_pointer_value_p (tree t) > return false; > else if (TYPE_PTRMEMFUNC_P (type)) > return (TREE_CODE (t) == CONSTRUCTOR > - && integer_zerop (CONSTRUCTOR_ELT (t, 0)->value)); > + && integer_zerop (fold (CONSTRUCTOR_ELT (t, 0)->value))); > else if (TYPE_PTRDATAMEM_P (type)) > - return integer_all_onesp (t); > + return integer_all_onesp (fold (t)); Calling fold here is wrong; it doesn't handle constexpr, and we should have folded before we got here. > warn_logical_operator (loc, code, boolean_type_node, > - code_orig_arg1, arg1, > - code_orig_arg2, arg2); > + code_orig_arg1, fold (arg1), > + code_orig_arg2, fold (arg2)); I think warn_logical_operator should call back into *_fully_fold. Likewise for most similar added calls to fold. > @@ -7356,8 +7354,13 @@ build_over_call (struct z_candidate *cand, int flags, tsu > bst_flags_t complain) > > gcc_assert (j <= nargs); > nargs = j; > + { > + tree *fargs = (!nargs ? argarray : (tree *) alloca (nargs * sizeof (tree))) > ; > + for (j = 0; j < nargs; j++) > + fargs[j] = fold_non_dependent_expr (argarray[j]); Similarly, this and build_cxx_call should use cp_fully_fold. > @@ -7602,7 +7614,6 @@ build_cxx_call (tree fn, int nargs, tree *argarray, > && current_function_decl > && DECL_DECLARED_CONSTEXPR_P (current_function_decl)) > optimize = 1; > - fn = fold_if_not_in_template (fn); > optimize = optimize_sav; Since we're removing the fold, we can also remove the changes to "optimize". > @@ -443,7 +443,7 @@ build_base_path (enum tree_code code, > > t = TREE_TYPE (TYPE_VFIELD (current_class_type)); > t = build_pointer_type (t); > - v_offset = convert (t, current_vtt_parm); > + v_offset = fold (convert (t, current_vtt_parm)); fold_convert should work here. > @@ -576,7 +576,6 @@ build_simple_base_path (tree expr, tree binfo) > expr = build3 (COMPONENT_REF, > cp_build_qualified_type (type, type_quals), > expr, field, NULL_TREE); > - expr = fold_if_not_in_template (expr); I don't think we need to remove this fold, since it is part of compiler internals rather than something the user wrote. Really, we should represent the base conversion with something like a CONVERT_EXPR and only call this function when we want to fold it. But that can wait for a later patch. > @@ -1046,6 +1048,9 @@ adjust_temp_type (tree type, tree temp) > { > if (TREE_TYPE (temp) == type) > return temp; > + STRIP_NOPS (temp); > + if (TREE_TYPE (temp) == type) > + return temp; ... > reduced_constant_expression_p (tree t) > { > + /* Make sure we remove useless initial NOP_EXPRs. */ > + STRIP_NOPS (t); Where are these NOPs coming from? > @@ -1082,7 +1087,10 @@ cxx_bind_parameters_in_call (const constexpr_ctx *ctx, tree t, > && is_dummy_object (x)) > { > x = ctx->object; > - x = cp_build_addr_expr (x, tf_warning_or_error); > + if (x) > + x = cp_build_addr_expr (x, tf_warning_or_error); > + else > + x = get_nth_callarg (t, i); This should not be necessary. > @@ -1765,7 +1780,8 @@ cxx_eval_component_reference (const constexpr_ctx *ctx, tree t, > if (field == part) > { > if (value) > - return value; > + return cxx_eval_constant_expression (ctx, value, lval, > + non_constant_p, overflow_p); ... > @@ -1849,7 +1865,8 @@ cxx_eval_bit_field_ref (const constexpr_ctx *ctx, tree t, > { > tree bitpos = bit_position (field); > if (bitpos == start && DECL_SIZE (field) == TREE_OPERAND (t, 1)) > - return value; > + return cxx_eval_constant_expression (ctx, value, lval, > + non_constant_p, overflow_p); This shouldn't be necessary, either; the elements of the CONSTRUCTOR should be fully evaluated already. > @@ -1560,14 +1570,19 @@ cxx_eval_unary_expression (const constexpr_ctx *ctx, tre > e t, > location_t loc = EXPR_LOCATION (t); > enum tree_code code = TREE_CODE (t); > tree type = TREE_TYPE (t); > - r = fold_unary_loc (loc, code, type, arg); > - if (r == NULL_TREE) > + if (TREE_CODE (t) == UNARY_PLUS_EXPR) > + r = fold_convert_loc (loc, TREE_TYPE (t), arg); We don't want to handle UNARY_PLUS_EXPR here; we should handle it like NOP_EXPR. And so you shouldn't need the call to unify_constant. > case BIT_NOT_EXPR: > case TRUTH_NOT_EXPR: > case FIXED_CONVERT_EXPR: > + case UNARY_PLUS_EXPR: > r = cxx_eval_unary_expression (ctx, t, lval, So this case should be down with NOP_EXPR. > @@ -2954,19 +2987,15 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, > tree t, > constexpr_ctx new_ctx; > tree r = t; > > - if (t == error_mark_node) > + if (!t || t == error_mark_node) Where are null expressions coming from? > case SIZEOF_EXPR: > + if (processing_template_decl > + && (!COMPLETE_TYPE_P (TREE_TYPE (t)) > + || TREE_CODE (TYPE_SIZE (TREE_TYPE (t))) != INTEGER_CST)) > + return t; The type of a SIZEOF_EXPR will always be size_t, so this isn't actually accomplishing anything, and should be removed. > + /* See this can happen for case like g++.dg/init/static2.C testcase. */ > + if (!ctx || !ctx->ctor || (lval && !ctx->object) > + || !same_type_ignoring_top_level_qualifiers_p > + (TREE_TYPE (t), TREE_TYPE (ctx->ctor)) > + || CONSTRUCTOR_NELTS (ctx->ctor) != 0) > + { > + *non_constant_p = true; > + break; > + } Why can this happen on the branch but not on trunk? I think the problem is elsewhere. > case NOP_EXPR: > { > tree oldop = TREE_OPERAND (t, 0); > + if (TREE_CODE (t) == NOP_EXPR && TREE_TYPE (t) == TREE_TYPE (oldop) && TREE_OVERFLOW_P (oldop)) > + { > + if (!ctx->quiet) > + permerror (input_location, "overflow in constant expression"); > + /* If we're being permissive (and are in an enforcing > + context), ignore the overflow. */ > + if (!flag_permissive) > + *overflow_p = true; > + *non_constant_p = true; > + > + return t; > + } This doesn't seem like the right place to handle this; why didn't we diagnose the overflow when it happened? > maybe_constant_init (tree t, tree decl) > { > + if (!t) > + return t; Where are null initializers coming from? > case MINUS_EXPR: > /* -- a subtraction where both operands are pointers. */ > if (TYPE_PTR_P (TREE_OPERAND (t, 0)) > - && TYPE_PTR_P (TREE_OPERAND (t, 1))) > + && TYPE_PTR_P (TREE_OPERAND (t, 1)) > + && TREE_OPERAND (t, 0) != TREE_OPERAND (t, 1)) Why? From where are we getting a pointer subtracted from itself? That said, we should probably just remove this case and the next, as they are obsolete. I'll remove them on the trunk. > +static tree > +cp_fold (tree x, hash_map *fold_hash) > +{ .... I still think we need a hybrid of this and the constexpr code: it isn't full folding if we aren't doing constexpr evaluation. But we can't just use maybe_constant_value because that only folds C++ constant-expressions, and we want to fold more things than that. I suppose one simple approach for now would be to call maybe_constant_value from cp_fold. > @@ -614,9 +614,13 @@ cp_fold_convert (tree type, tree expr) > } > else > { > - conv = fold_convert (type, expr); > + if (TREE_CODE (expr) == INTEGER_CST) > + conv = fold_convert (type, expr); > + else > + conv = convert (type, expr); Why? If we're calling cp_fold_convert in a place where we don't want to fold, we should stop calling it rather than change it. > cp_convert_and_check (tree type, tree expr, tsubst_flags_t complain) > { > - tree result; > + tree result, ret; > > if (TREE_TYPE (expr) == type) > return expr; > > - result = cp_convert (type, expr, complain); > + result = ret = cp_convert (type, expr, complain); > > if ((complain & tf_warning) > && c_inhibit_evaluation_warnings == 0) > @@ -652,6 +656,7 @@ cp_convert_and_check (tree type, tree expr, tsubst_flags_t complain) > tree stripped = folded; > tree folded_result > = folded != expr ? cp_convert (type, folded, complain) : result; > + folded_result = fold (folded_result); > > /* maybe_constant_value wraps an INTEGER_CST with TREE_OVERFLOW in a > NOP_EXPR so that it isn't TREE_CONSTANT anymore. */ > @@ -663,7 +668,7 @@ cp_convert_and_check (tree type, tree expr, tsubst_flags_t complain) > folded_result); > } > > - return result; > + return ret; Why introduce the "ret" variable? It doesn't seem to do anything different from "result". And instead of the added fold, maybe change the cp_convert on the previous line to cp_fold_convert? > @@ -1535,8 +1538,10 @@ build_expr_type_conversion (int desires, tree expr, bool complain) > + tree expr_folded = maybe_constant_value (expr); > > - if (expr == null_node > + STRIP_NOPS (expr_folded); > + if (expr_folded == null_node We shouldn't need to fold to check for null_node, it only occurs when explicitly written. And we don't want to fold before calling null_ptr_cst_p, since in C++11 only a literal 0 is a null pointer constant. For C++98 we already fold in null_ptr_cst_p. > @@ -8502,16 +8502,18 @@ compute_array_index_type (tree name, tree size, tsubst_flags_t complain) > + /* We need to do fully folding to determine if we have VLA, or not. */ > + tree size_constant = maybe_constant_value (size); Why is this needed? We already call maybe_constant_value earlier in compute_array_index_type. > - itype = fold (itype); > + itype = maybe_constant_value (itype); > - itype = variable_size (fold (newitype)); > + itype = variable_size (maybe_constant_value (newitype)); Maybe these should use cp_fully_fold? > @@ -13090,6 +13092,8 @@ build_enumerator (tree name, tree value, tree enumtype, location_t loc) > + if (value) > + value = maybe_constant_value (value); This seems unnecessary, since we call cxx_constant_value below. > value = cxx_constant_value (value); > + STRIP_NOPS (value); The only time a constant result should have a NOP_EXPR around it is if it isn't really constant. Why do you want to strip that? > - value = convert (ENUM_UNDERLYING_TYPE (enumtype), value); > + value = fold (convert (ENUM_UNDERLYING_TYPE (enumtype), value)); fold_convert again. > @@ -188,9 +188,9 @@ build_zero_init_1 (tree type, tree nelts, bool static_storage_p, > - init = convert (type, nullptr_node); > + init = fold (convert (type, nullptr_node)); fold_convert > @@ -783,7 +783,8 @@ perform_member_init (tree member, tree init) > + if (init) > + init = fold (init); Why fold here? This doesn't seem like a place that needs early folding. > @@ -6480,7 +6480,8 @@ cp_parser_array_notation (location_t loc, cp_parser *parser, tree *init_index, > - *init_index = cp_parser_expression (parser); > + *init_index = cp_parser_expression (parser); > + *init_index = maybe_constant_value (*init_index); ... > + length_index = maybe_constant_value (length_index); ... > + stride = maybe_constant_value (stride); Why fold here, rather than later when something really wants a constant? If that ever actually occurs? > + /* For offsetof and declaration of types we need > + constant integeral values. > + Also we meed to fold for negative constants so that diagnostic in > + c-family/c-common.c doesn't fail for array-bounds. */ > + if (for_offsetof || decltype_p > + || (TREE_CODE (index) == NEGATE_EXPR && TREE_CODE (TREE_OPERAND (index, 0)) == INTEGER_CST)) > + index = maybe_constant_value (index); Likewise. For offsetof we should either use OFFSETOF_EXPR until late folding, or fold in offsetof evaluation. I don't know why decltype would need anything special. And for diagnostics we should be folding closer to the diagnostic. > @@ -9876,6 +9888,7 @@ cp_parser_label_for_labeled_statement (cp_parser* parser, tree attributes) > + expr = maybe_constant_value (expr); This seems redundant with the call to cxx_constant_value in case_conversion. > @@ -12190,6 +12204,10 @@ cp_parser_static_assert(cp_parser *parser, bool member_p) > + /* Make sure we folded it completely before doing trying to get > + constant value. */ > + condition = fold_non_dependent_expr (condition); This shouldn't be necessary; if the constexpr code needs to do more folding, that should be fixed. > @@ -16081,6 +16099,7 @@ cp_parser_enumerator_definition (cp_parser* parser, tree type) > + value = maybe_constant_value (value); This seems redundant with the call to cxx_constant_value in build_enumerator. > + width = maybe_constant_value (width); This seems redundant with the call to cxx_constant_value in check_bitfield_decl. And so on. It seems like you added maybe_constant_value after every occurrence of cp_parser_constant_expression, and I suspect that few are actually needed, and the ones that are should go closer to the code that really needs a constant. I'd prefer to avoid calling it at all in parser.c. > @@ -449,7 +449,7 @@ build_aggr_init_expr (tree type, tree init) > - return convert (type, init); > + return fold (convert (type, init)); fold_convert > @@ -3394,6 +3394,8 @@ handle_init_priority_attribute (tree* node, > + if (initp_expr) > + initp_expr = maybe_constant_value (initp_expr); Let's use cxx_constant_value instead of this and the non-constant diagnostic just below. > @@ -3371,7 +3367,7 @@ get_member_function_from_ptrfunc (tree *instance_ptrptr, tree function, > - e2 = fold_convert (TREE_TYPE (e3), e2); > + e2 = fold (convert (TREE_TYPE (e3), e2)); Why? > @@ -3667,6 +3663,10 @@ convert_arguments (tree typelist, vec **valu > + /* For BUILT_IN_NORMAL we want to fold constants. */ > + if (fndecl && DECL_BUILT_IN (fndecl) > + && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL) > + val = fold (val); This should be cp_fully_fold, and lower down, after all the conversions. > - tree xop0 = op0, xop1 = op1, xresult_type = result_type; > + tree xop0 = fold (op0), xop1 = fold (op1), xresult_type = result_type; This seems wrong. In fact, the whole short_compare business seems like the sort of early folding we want to do away with. > - if (TREE_OVERFLOW_P (result) > + op0 = fold_non_dependent_expr (op0); > + op1 = fold_non_dependent_expr (op1); > + STRIP_NOPS (op0); > + STRIP_NOPS (op1); > + result_ovl = fold_build2 (resultcode, build_type, op0, op1); > + if (TREE_OVERFLOW_P (result_ovl) > && !TREE_OVERFLOW_P (op0) > && !TREE_OVERFLOW_P (op1)) > - overflow_warning (location, result); > + overflow_warning (location, result_ovl); What if we don't try to fold for this warning early, and instead give the warning later when we're folding? I suppose that might apply to lots of the warnings that we're currently folding early for. > @@ -7983,7 +7978,6 @@ expand_ptrmemfunc_cst (tree cst, tree *delta, tree *pfn) > tree binfo = binfo_or_else (orig_class, fn_class); > *delta = build2 (PLUS_EXPR, TREE_TYPE (*delta), > *delta, BINFO_OFFSET (binfo)); > - *delta = fold_if_not_in_template (*delta); I think all the calls to fold_if_not_in_template in expand_ptrmemfunc_cst should become regular folds. Or rather, change the build2 to fold_build2. This is very much compiler internals, and we should only get here when folding anyway. > - gcc_assert (val1->v.val_unsigned == DWARF2_ADDR_SIZE); > + gcc_assert (val1->v.val_unsigned > + == (unsigned HOST_WIDE_INT) DWARF2_ADDR_SIZE); We need to fix this warning so this change is unnecessary. > - gcc_assert (TREE_OPERAND (t, 0) == decl); > + gcc_assert (TREE_OPERAND (t, 0) == decl || TREE_OPERAND (t, 1) == decl); This change doesn't seem to have anything to do with delayed folding. > || (gimple_omp_for_kind (for_stmt) > - == GF_OMP_FOR_KIND_CILKFOR)); > + == GF_OMP_FOR_KIND_CILKFOR) > + || (gimple_omp_for_kind (for_stmt) > + == GF_OMP_FOR_KIND_FOR)); Nor this one. > +++ b/gcc/testsuite/g++.dg/ext/offsetof1.C > +// { dg-options "-Wno-pointer-arith" } There isn't any user-written pointer arithmetic in this testcase, so any such warnings are bogus. > +++ b/gcc/testsuite/g++.dg/warn/Wconversion-pr34389.C > @@ -50,5 +50,5 @@ short mask5(int x) > > short mask6(short x) > { > - return x & -1; > + return x & -1; // { dg-warning "conversion" } This is also a false positive. > +++ b/gcc/testsuite/g++.dg/warn/skip-1.C > -// Check that we don't warn about code that will not be executed. > +// For delayed folding we will warn about code that will not be executed too. This is not an improvement. > @@ -1791,6 +1791,9 @@ evaluate_stmt (gimple stmt) > && (likelyvalue == CONSTANT || is_gimple_call (stmt) > || (gimple_assign_single_p (stmt) > && gimple_assign_rhs_code (stmt) == ADDR_EXPR)) > + && (likelyvalue == CONSTANT || is_gimple_call (stmt) > + || (gimple_assign_single_p (stmt) > + && gimple_assign_rhs_code (stmt) == ADDR_EXPR)) Merge error? > @@ -1956,6 +1956,8 @@ build_complex (tree type, tree real, tree imag) > { > tree t = make_node (COMPLEX_CST); > > + real = fold (real); > + imag = fold (imag); I don't think we want to introduce folding into language-independent code like here. > @@ -5062,6 +5063,7 @@ output_constructor_bitfield (oc_local_state *local, unsigned int bit_offset) > while (TREE_CODE (local->val) == VIEW_CONVERT_EXPR > || TREE_CODE (local->val) == NON_LVALUE_EXPR) > local->val = TREE_OPERAND (local->val, 0); > + local->val = fold (local->val); Or here. Jason