C++ delayed folding branch review

[Jason Merrill <jason@redhat.com>]

> +  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