This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
[c++-delayed-folding] code review
- From: Jason Merrill <jason at redhat dot com>
- To: Kai Tietz <ktietz70 at googlemail dot com>
- Cc: gcc-patches List <gcc-patches at gcc dot gnu dot org>
- Date: Thu, 17 Sep 2015 02:18:01 -0400
- Subject: [c++-delayed-folding] code review
- Authentication-results: sourceware.org; auth=none
@@ -216,6 +216,7 @@ libgcov-driver-tool.o-warn = -Wno-error
libgcov-merge-tool.o-warn = -Wno-error
gimple-match.o-warn = -Wno-unused
generic-match.o-warn = -Wno-unused
+insn-modes.o-warn = -Wno-error
This doesn't seem to be needed anymore.
@@ -397,11 +397,13 @@ convert_to_real (tree type, tree expr)
EXPR must be pointer, integer, discrete (enum, char, or bool), float,
fixed-point or vector; in other cases error is called.
+ If DOFOLD is TRZE, we try to simplify newly-created patterns by folding.
"TRUE"
+ if (!dofold)
+ {
+ expr = build1 (CONVERT_EXPR,
+ lang_hooks.types.type_for_size
+ (TYPE_PRECISION (intype), 0),
+ expr);
+ return build1 (CONVERT_EXPR, type, expr);
+ }
When we're not folding, I don't think we want to do the two-step
conversion, just the second one. And we might want to use NOP_EXPR
instead of CONVERT_EXPR, but I'm not sure about that.
@@ -818,10 +828,15 @@ convert_to_integer (tree type, tree expr)
if (TYPE_UNSIGNED (typex))
typex = signed_type_for (typex);
}
- return convert (type,
- fold_build2 (ex_form, typex,
- convert (typex, arg0),
- convert (typex, arg1)));
+ if (dofold)
+ return convert (type,
+ fold_build2 (ex_form, typex,
+ convert (typex, arg0),
+ convert (typex, arg1)));
+ arg0 = build1 (CONVERT_EXPR, typex, arg0);
+ arg1 = build1 (CONVERT_EXPR, typex, arg1);
+ expr = build2 (ex_form, typex, arg0, arg1);
+ return build1 (CONVERT_EXPR, type, expr);
This code path seems to be for pushing a conversion down into a binary
expression. We shouldn't do this at all when we aren't folding.
@@ -845,9 +860,14 @@ convert_to_integer (tree type, tree expr)
if (!TYPE_UNSIGNED (typex))
typex = unsigned_type_for (typex);
+ if (!dofold)
+ return build1 (CONVERT_EXPR, type,
+ build1 (ex_form, typex,
+ build1 (CONVERT_EXPR, typex,
+ TREE_OPERAND (expr, 0))));
Likewise.
@@ -867,6 +887,14 @@ convert_to_integer (tree type, tree expr)
the conditional and never loses. A COND_EXPR may have a throw
as one operand, which then has void type. Just leave void
operands as they are. */
+ if (!dofold)
+ return build3 (COND_EXPR, type, TREE_OPERAND (expr, 0),
+ VOID_TYPE_P (TREE_TYPE (TREE_OPERAND (expr, 1)))
+ ? TREE_OPERAND (expr, 1)
+ : build1 (CONVERT_EXPR, type, TREE_OPERAND (expr, 1)),
+ VOID_TYPE_P (TREE_TYPE (TREE_OPERAND (expr, 2)))
+ ? TREE_OPERAND (expr, 2)
+ : build1 (CONVERT_EXPR, type, TREE_OPERAND (expr, 2)));
Likewise.
@@ -903,6 +933,10 @@ convert_to_integer (tree type, tree expr)
return build1 (FIXED_CONVERT_EXPR, type, expr);
case COMPLEX_TYPE:
+ if (!dofold)
+ return build1 (CONVERT_EXPR, type,
+ build1 (REALPART_EXPR,
+ TREE_TYPE (TREE_TYPE (expr)), expr));
Why can't we call convert here rather than build1 a CONVERT_EXPR?
It would be good to ask a fold/convert maintainer to review the changes
to this file, too.
@@ -5671,8 +5668,8 @@ build_new_op_1 (location_t loc, enum tree_code code, int flags, tree arg1,
decaying an enumerator to its value. */
if (complain & tf_warning)
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));
arg2 = convert_like (conv, arg2, complain);
}
@@ -5710,7 +5707,7 @@ build_new_op_1 (location_t loc, enum tree_code code, int flags, tree arg1,
case TRUTH_OR_EXPR:
if (complain & tf_warning)
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));
/* Fall through. */
case GT_EXPR:
case LT_EXPR:
@@ -5721,9 +5718,10 @@ build_new_op_1 (location_t loc, enum tree_code code, int flags, tree arg1,
if ((complain & tf_warning)
&& ((code_orig_arg1 == BOOLEAN_TYPE)
^ (code_orig_arg2 == BOOLEAN_TYPE)))
- maybe_warn_bool_compare (loc, code, arg1, arg2);
+ maybe_warn_bool_compare (loc, code, fold (arg1),
+ fold (arg2));
if (complain & tf_warning && warn_tautological_compare)
- warn_tautological_cmp (loc, code, arg1, arg2);
+ warn_tautological_cmp (loc, code, fold (arg1), fold (arg2));
Would it make sense to change all these folds to maybe_constant_value?
@@ -7433,8 +7431,13 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_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]);
- check_function_arguments (TREE_TYPE (fn), nargs, argarray);
+ check_function_arguments (TREE_TYPE (fn), nargs, fargs);
+ }
Using fold_non_dependent_expr is wrong here; we can't be in a template
at this point, so it is equivalent to maybe_constant_value. We might
also avoid folding entirely if !warn_nonnull.
@@ -428,7 +428,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));
This case should be fine with fold_convert: we're changing from one
pointer type to another.
@@ -2538,12 +2551,19 @@ cxx_eval_indirect_ref (const constexpr_ctx *ctx, tree t,
bool lval,
bool *non_constant_p, bool *overflow_p)
{
- tree orig_op0 = TREE_OPERAND (t, 0);
+ tree r, orig_op0 = TREE_OPERAND (t, 0);
bool empty_base = false;
+ if (TREE_CODE (t) == MEM_REF
+ && (!TREE_OPERAND (t, 1) || !integer_zerop (TREE_OPERAND (t, 1))))
+ {
+ *non_constant_p = true;
+ return t;
+ }
+
/* First try to simplify it directly. */
- tree r = cxx_fold_indirect_ref (EXPR_LOCATION (t), TREE_TYPE (t), orig_op0,
- &empty_base);
+ r = cxx_fold_indirect_ref (EXPR_LOCATION (t), TREE_TYPE (t), orig_op0,
+ &empty_ba
I don't see a reason for the change to where 'r' is declared.
Please add a comment about MEM_REF.
@@ -3347,6 +3372,8 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t,
/* Don't VERIFY_CONSTANT here. */
if (*non_constant_p)
return t;
+ if (TREE_CODE (op) == CONSTRUCTOR)
+ return t;
gcc_checking_assert (TREE_CODE (op) != CONSTRUCTOR);
This is wrong.
@@ -3370,6 +3397,10 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t,
break;
case SIZEOF_EXPR:
+ if (processing_template_decl
+ && (!COMPLETE_TYPE_P (TREE_TYPE (t))
+ || TREE_CODE (TYPE_SIZE (TREE_TYPE (t))) != INTEGER_CST))
+ return t;
This is still wrong.
@@ -3651,6 +3689,9 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t,
non_constant_p, overflow_p, jump_target);
break;
+ case ARRAY_NOTATION_REF:
+ break;
This seems unnecessary; an ARRAY_NOTATION_REF shouldn't get this far.
+/* Helper routine for fold_simplet function. Either return simplified
s/simplet/simple/
+static tree
+cp_fold_r (tree *stmt_p, int *walk_subtrees ATTRIBUTE_UNUSED, void *data)
walk_subtrees is used now.
Also, this function needs more comments.
+static tree
+cp_fold (tree x, hash_map<tree, tree> *fold_hash)
This also needs a lot more comments.
@@ -642,13 +645,14 @@ cp_convert_and_check (tree type, tree expr, tsubst_flags_t
complain)
{
tree folded = maybe_constant_value (expr);
tree stripped = folded;
- tree folded_result
+ tree folded_result;
+ folded_result
= folded != expr ? cp_convert (type, folded, complain) : result;
-
- /* maybe_constant_value wraps an INTEGER_CST with TREE_OVERFLOW in a
- NOP_EXPR so that it isn't TREE_CONSTANT anymore. */
+ folded_result = cp_fully_fold (folded_result);
+ /* The maybe_constant_value wraps an INTEGER_CST with TREE_OVERFLOW
+ in a NOP_EXPR so that it isn't TREE_CONSTANT anymore. */
STRIP_NOPS (stripped);
-
+ folded = cp_fully_fold (folded);
Now that we're caching maybe_constant_value, I think we can replace most
of the calls to cp_fully_fold with either maybe_constant_value or
fold_simple.
Here, we already called maybe_constant_value, so there's no need to fold
folded again. And instead of folding folded_result, we should be able
to replace the cp_convert with cp_fold_convert.
@@ -3362,6 +3362,11 @@ finish_case_label (location_t loc, tree low_value, tree high_value)
{
tree label;
+ /* For build_case_label we need to make sure that arguments
+ got fully folded. */
+ low_value = cp_fully_fold (low_value);
+ high_value = cp_fully_fold (high_value);
Looks like you removed another set of folds in this function, but this
one still needs to go away.
@@ -6555,12 +6561,13 @@ cp_finish_decl (tree decl, tree init, bool init_const_expr_p,
if (init && VAR_P (decl))
{
+ init_cst = cp_fully_fold (init);
DECL_NONTRIVIALLY_INITIALIZED_P (decl) = 1;
/* If DECL is a reference, then we want to know whether init is a
reference constant; init_const_expr_p as passed tells us whether
it's an rvalue constant. */
if (TREE_CODE (type) == REFERENCE_TYPE)
- init_const_expr_p = potential_constant_expression (init);
+ init_const_expr_p = potential_constant_expression (init_cst);
if (init_const_expr_p)
{
/* Set these flags now for templates. We'll update the flags in
@@ -6622,10 +6629,11 @@ cp_finish_decl (tree decl, tree init, bool init_const_expr_p,
&& !MAYBE_CLASS_TYPE_P (type))
init = build_x_compound_expr_from_list (init, ELK_INIT,
tf_warning_or_error);
+ init_cst = init;
}
if (init)
- DECL_INITIAL (decl) = init;
+ DECL_INITIAL (decl) = init_cst;
return;
}
We don't want this folding here in cp_finish_decl; we agreed before to
fold initializers, but I think the right place to do this is where you
already added it to store_init_value, not here.
@@ -8674,16 +8653,18 @@ compute_array_index_type (tree name, tree size, tsubst_flags_t complain)
SET_TYPE_STRUCTURAL_EQUALITY (itype);
return itype;
}
-
+
+ tree size_constant = cp_fully_fold (size);
I've been saying all along that this is redundant.
@@ -3493,6 +3495,8 @@ build_vec_init (tree base, tree maxindex, tree init,
}
maxindex = cp_convert (ptrdiff_type_node, maxindex, complain);
+ maxindex = maybe_constant_value (maxindex);
I think this can be fold_simple.
@@ -6648,6 +6648,11 @@ cp_parser_postfix_open_square_expression (cp_parser *parser,
index = cp_parser_expression (parser);
}
+ /* For offsetof and declaration of types we need
+ constant integeral values.
+ Also we need to fold for negative constants so that diagnostic in
+ c-family/c-common.c doesn't fail for array-bounds. */
+ index = fold_simple (index);
This continues to be the wrong place for this.
@@ -8124,7 +8129,9 @@ cp_parser_cast_expression (cp_parser *parser, bool address_p, bool cast_p,
return error_mark_node;
/* Perform the cast. */
- expr = build_c_cast (input_location, type, expr);
+ /* We don't want to resolve cast too early. Therefore we don't
+ be able to use build_c_cast. */
+ expr = cp_build_c_cast (type, expr, tf_warning_or_error);
And this change still has no effect.
@@ -8286,9 +8293,11 @@ cp_parser_binary_expression (cp_parser* parser, bool cast
_p,
/* For "false && x" or "true || x", x will never be executed;
disable warnings while evaluating it. */
if (current.tree_type == TRUTH_ANDIF_EXPR)
- c_inhibit_evaluation_warnings += current.lhs == truthvalue_false_node;
+ c_inhibit_evaluation_warnings +=
+ cp_fully_fold (current.lhs) == truthvalue_false_node;
else if (current.tree_type == TRUTH_ORIF_EXPR)
- c_inhibit_evaluation_warnings += current.lhs == truthvalue_true_node;
+ c_inhibit_evaluation_warnings +=
+ cp_fully_fold (current.lhs) == truthvalue_true_node;
/* Extract another operand. It may be the RHS of this expression
or the LHS of a new, higher priority expression. */
@@ -8332,9 +8341,11 @@ cp_parser_binary_expression (cp_parser* parser, bool cast_p,
/* Undo the disabling of warnings done above. */
if (current.tree_type == TRUTH_ANDIF_EXPR)
- c_inhibit_evaluation_warnings -= current.lhs == truthvalue_false_node;
+ c_inhibit_evaluation_warnings -=
+ cp_fully_fold (current.lhs) == truthvalue_false_node;
else if (current.tree_type == TRUTH_ORIF_EXPR)
- c_inhibit_evaluation_warnings -= current.lhs == truthvalue_true_node;
+ c_inhibit_evaluation_warnings -=
+ cp_fully_fold (current.lhs) == truthvalue_true_node;
if (warn_logical_not_paren
&& TREE_CODE_CLASS (current.tree_type) == tcc_comparison
@@ -8421,7 +8432,7 @@ cp_parser_binary_expression (cp_parser* parser, bool cast_p,
static tree
cp_parser_question_colon_clause (cp_parser* parser, tree logical_or_expr)
{
- tree expr;
{
- tree expr;
+ tree expr, folded_logical_or_expr = cp_fully_fold (logical_or_expr);
tree assignment_expr;
struct cp_token *token;
location_t loc = cp_lexer_peek_token (parser->lexer)->location;
@@ -8436,7 +8447,8 @@ cp_parser_question_colon_clause (cp_parser* parser, tree logical_or_expr)
"ISO C++ does not allow ?: with omitted middle operand");
/* Implicit true clause. */
expr = NULL_TREE;
- c_inhibit_evaluation_warnings += logical_or_expr == truthvalue_true_node;
- c_inhibit_evaluation_warnings += current.lhs == truthvalue_true_node;
+ c_inhibit_evaluation_warnings +=
+ cp_fully_fold (current.lhs) == truthvalue_true_node;
/* Extract another operand. It may be the RHS of this expression
or the LHS of a new, higher priority expression. */
@@ -8332,9 +8341,11 @@ cp_parser_binary_expression (cp_parser* parser, bool cast
_p,
/* Undo the disabling of warnings done above. */
if (current.tree_type == TRUTH_ANDIF_EXPR)
- c_inhibit_evaluation_warnings -= current.lhs == truthvalue_false_node;
+ c_inhibit_evaluation_warnings -=
+ cp_fully_fold (current.lhs) == truthvalue_false_node;
else if (current.tree_type == TRUTH_ORIF_EXPR)
- c_inhibit_evaluation_warnings -= current.lhs == truthvalue_true_node;
+ c_inhibit_evaluation_warnings -=
+ cp_fully_fold (current.lhs) == truthvalue_true_node;
if (warn_logical_not_paren
&& TREE_CODE_CLASS (current.tree_type) == tcc_comparison
@@ -8421,7 +8432,7 @@ cp_parser_binary_expression (cp_parser* parser, bool cast_
p,
static tree
cp_parser_question_colon_clause (cp_parser* parser, tree logical_or_expr)
{
- tree expr;
+ tree expr, folded_logical_or_expr = cp_fully_fold (logical_or_expr);
These should change to maybe_constant_value.
@@ -12311,6 +12325,11 @@ cp_parser_static_assert(cp_parser *parser, bool member_p)
/*allow_non_constant_p=*/true,
/*non_constant_p=*/&dummy);
+ /* Reduce condition early. We need to reduce builtins within
+ static_asserts, so that testcase like pr62024.C getting
+ resolved always proper. */
+ condition = fold_non_dependent_expr (condition);
If pr62024.C isn't handled properly by maybe_constant_value, that should
be fixed rather than worked around here.
@@ -28920,6 +28940,11 @@ cp_parser_omp_var_list_no_open (cp_parser *parser, enum omp_clause_code kind,
CPP_CLOSE_SQUARE))
length = cp_parser_expression (parser);
}
+ /* So we need here fully folded values. */
+ if (length)
+ length = cp_fully_fold (length);
+ if (low_bound)
+ low_bound = cp_fully_fold (low_bound);
I think these can change to maybe_constant_value.
@@ -31276,6 +31302,8 @@ cp_parser_omp_for_incr (cp_parser *parser, tree decl)
else
lhs = build_x_unary_op (input_location, NEGATE_EXPR, rhs,
tf_warning_or_error);
+ if (op != PLUS_EXPR && CONSTANT_CLASS_P (rhs))
+ lhs = fold (lhs);
This should be fold_simple.
@@ -29856,6 +29881,7 @@ cp_parser_omp_clause_aligned (cp_parser *parser, tree li
st)
if (colon)
{
alignment = cp_parser_constant_expression (parser);
+ alignment = alignment;
This seems unnecessary. :)
@@ -4616,10 +4612,12 @@ cp_build_binary_op (location_t location,
op1 = save_expr (op1);
pfn0 = pfn_from_ptrmemfunc (op0);
+ pfn0 = cp_fully_fold (pfn0);
/* Avoid -Waddress warnings (c++/64877). */
if (TREE_CODE (pfn0) == ADDR_EXPR)
TREE_NO_WARNING (pfn0) = 1;
pfn1 = pfn_from_ptrmemfunc (op1);
+ pfn1 = cp_fully_fold (pfn1);
I'm not sure what the fold here is for. Your checkin message suggests
that it's there to expose the ADDR_EXPR so we can set TREE_NO_WARNING on
it. I think it would be better to set TREE_NO_WARNING when we call
build_addr_func in expand_ptrmemfunc_cst rather than here.
@@ -4992,9 +4992,9 @@ cp_build_binary_op (location_t location,
tree oop1 = maybe_constant_value (orig_op1);
if (TREE_CODE (oop0) != INTEGER_CST)
- oop0 = orig_op0;
+ oop0 = cp_fully_fold (orig_op0);
if (TREE_CODE (oop1) != INTEGER_CST)
- oop1 = orig_op1;
+ oop1 = cp_fully_fold (orig_op1);
If we're going to need cp_fully_fold here, we should call it instead of
maybe_constant_value rather than call both functions.
@@ -882,6 +884,7 @@ check_narrowing (tree type, tree init, tsubst_flags_t complain)
}
init = fold_non_dependent_expr (init);
+ init = cp_fully_fold (init);
What is this for?
@@ -1859,7 +1859,8 @@ output_loc_operands (dw_loc_descr_ref loc, int for_eh_or_skip)
}
break;
case dw_val_class_addr:
- gcc_assert (val1->v.val_unsigned == DWARF2_ADDR_SIZE);
+ gcc_assert (val1->v.val_unsigned
+ == (unsigned HOST_WIDE_INT) DWARF2_ADDR_SIZE);
This shouldn't be necessary anymore.
@@ -7294,7 +7294,7 @@ gimplify_omp_for (tree *expr_p, gimple_seq *pre_p)
/* Handle OMP_FOR_COND. */
t = TREE_VEC_ELT (OMP_FOR_COND (for_stmt), i);
gcc_assert (COMPARISON_CLASS_P (t));
- gcc_assert (TREE_OPERAND (t, 0) == decl);
+ gcc_assert (TREE_OPERAND (t, 0) == decl || TREE_OPERAND (t, 1) == decl);
Nor this, now that we're folding the OMP condition.
@@ -7872,8 +7872,7 @@ get_delta_difference (tree from, tree to,
}
}
- return fold_if_not_in_template (convert_to_integer (ptrdiff_type_node,
- result));
+ return convert_to_integer (ptrdiff_type_node, result);
}
/* Return a constructor for the pointer-to-member-function TYPE using
@@ -8056,7 +8055,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);
/* We set PFN to the vtable offset at which the function can be
found, plus one (unless ptrmemfunc_vbit_in_delta, in which
@@ -8064,23 +8062,19 @@ expand_ptrmemfunc_cst (tree cst, tree *delta, tree *pfn)
*pfn = DECL_VINDEX (fn);
*pfn = build2 (MULT_EXPR, integer_type_node, *pfn,
TYPE_SIZE_UNIT (vtable_entry_type));
- *pfn = fold_if_not_in_template (*pfn);
switch (TARGET_PTRMEMFUNC_VBIT_LOCATION)
{
case ptrmemfunc_vbit_in_pfn:
*pfn = build2 (PLUS_EXPR, integer_type_node, *pfn,
integer_one_node);
- *pfn = fold_if_not_in_template (*pfn);
break;
case ptrmemfunc_vbit_in_delta:
*delta = build2 (LSHIFT_EXPR, TREE_TYPE (*delta),
*delta, integer_one_node);
- *delta = fold_if_not_in_template (*delta);
*delta = build2 (PLUS_EXPR, TREE_TYPE (*delta),
*delta, integer_one_node);
- *delta = fold_if_not_in_template (*delta);
break;
default:
@@ -8088,7 +8082,6 @@ expand_ptrmemfunc_cst (tree cst, tree *delta, tree *pfn)
}
*pfn = build_nop (TYPE_PTRMEMFUNC_FN_TYPE (type), *pfn);
- *pfn = fold_if_not_in_template (*pfn);
I still think we want to keep these folds, since the trees are all
compiler generated.
@@ -1859,7 +1859,8 @@ output_loc_operands (dw_loc_descr_ref loc, int for_eh_or_s
kip)
}
break;
case dw_val_class_addr:
- gcc_assert (val1->v.val_unsigned == DWARF2_ADDR_SIZE);
+ gcc_assert (val1->v.val_unsigned
+ == (unsigned HOST_WIDE_INT) DWARF2_ADDR_SIZE);
This isn't needed anymore.
@@ -7294,7 +7294,7 @@ gimplify_omp_for (tree *expr_p, gimple_seq *pre_p)
/* Handle OMP_FOR_COND. */
t = TREE_VEC_ELT (OMP_FOR_COND (for_stmt), i);
gcc_assert (COMPARISON_CLASS_P (t));
- gcc_assert (TREE_OPERAND (t, 0) == decl);
+ gcc_assert (TREE_OPERAND (t, 0) == decl || TREE_OPERAND (t, 1) == decl);
And this shouldn't be, either.
Jason