This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
C++ delayed folding branch 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: Fri, 24 Apr 2015 00:22:49 -0400
- Subject: C++ delayed folding branch review
- Authentication-results: sourceware.org; auth=none
+ expr = fold (expr);
/* This may happen, because for LHS op= RHS we preevaluate
RHS and create C_MAYBE_CONST_EXPR <SAVE_EXPR <RHS>>, 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<tree, tree> *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<tree, va_gc> **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