Re: C++ delayed folding branch review

[Jason Merrill <jason@redhat.com>]

On 06/12/2015 12:11 PM, Kai Tietz wrote:
>>> @@ -589,9 +589,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));
>>
>> Again, calling fold here is wrong; it doesn't handle constexpr, and we
>> should have folded before we got here.
>
> Agreed.  I will commit change for this.
>
> Nevertheless CONSTRUCTOR_ELT's value might still be prefixed by nops due possible overflows, or by negative sign/invert/etc.

It shouldn't in any calls to this function; the argument to this 
function should have already been folded.

>>> @@ -5090,9 +5090,9 @@ build_conditional_expr_1 (location_t loc, tree arg1,
>>> tree arg2, tree arg3,
>>>
>>>    valid_operands:
>>>     result = build3 (COND_EXPR, result_type, arg1, arg2, arg3);
>>> -  if (!cp_unevaluated_operand)
>>> +  if (!cp_unevaluated_operand && !processing_template_decl)
>>>       /* Avoid folding within decltype (c++/42013) and noexcept.  */
>>> -    result = fold_if_not_in_template (result);
>>> +    result = fold (result);
>>
>> This seems related to your status report note:
>>
>>> Additionally addressed issue about cond_expr, as we want to fold cases with
>>> a constant-condition.  Here we need to use "fold_to_constant" so that we
>>> just fold things to constant-value, if possible and otherwise don't modify
>>> anything.
>>
>> But why do you say we want to fold cases with a constant condition?  We
>> certainly want to avoid warning about the dead branch in that case, but
>> it would be better if we can do that folding only in the warning code.
>
> Issue is that we otherwise detect in conditions that expressions aren't constant due never-executed-code-path.

How so?  The code for determining whether an expression is constant 
should do the folding.

I think the way to avoid warnings about dead code paths is to do the 
folding in cp_parser_question_colon_clause and in tsubst_copy_and_build, 
case COND_EXPR.

> The diagnostics we can deal differently, but this was actually the reason for doing this.  I can remove this here, but we still need a place to avoid ill detection of constexpr (or invalid code) on dead code-branch.  Eg.  (1 ? 0/0 : 1) etc

>>> @@ -7382,8 +7383,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]);
>>
>> No change needed here, but I notice that fold_non_dependent_expr is
>> still using maybe_constant_value; it should probably use cp_fully_fold
>> instead.
>
> Hmm, maybe we should limit this folding on constants.  So cp_fold_to_constant ()?

This folding is just for diagnostics, so I think cp_fully_fold is the 
right choice.

>>> @@ -1052,6 +1054,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;
>>> @@ -1430,6 +1438,8 @@ cxx_eval_call_expression (const constexpr_ctx *ctx,
>>> tree t,
>>>   bool
>>>   reduced_constant_expression_p (tree t)
>>>   {
>>> +  /* Make sure we remove useless initial NOP_EXPRs.  */
>>> +  STRIP_NOPS (t);
>>
>> Within the constexpr code we should be folding away NOPs as they are
>> generated, they shouldn't live this long.
>
> Well, we might see them on overflows ...

We shouldn't within the constexpr code.  NOPs for expressions that are 
non-constant due to overflow are added in 
cxx_eval_outermost_constant_expr, so we shouldn't see them in the middle 
of constexpr evaluation.

>>> @@ -1088,7 +1093,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 still should not be necessary.
>
> Yeah, most likely.  But I got initially here some issues, so I don't see that this code would worsen things.

If this code path is hit, that means something has broken my design, and 
I don't want to just paper over that.  Please revert this change.

>>> @@ -1576,13 +1586,15 @@ cxx_eval_unary_expression (const constexpr_ctx
>>> *ctx, tre
>>> e 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 (r == NULL_TREE || !CONSTANT_CLASS_P (r))
>>>       {
>>>         if (arg == orig_arg)
>>>          r = t;
>>>         else
>>>          r = build1_loc (loc, code, type, arg);
>>>       }
>>> +  else
>>> +    r = unify_constant (ctx, r, overflow_p);
>>
>> This still should not be necessary.
>
> Well, I just wanted to make sure that if arg is a PTRMEM_CST (or something like this) that we still try to resolve its constant.  But yes, you are right that arg is anyway resolved already, and so this might not happen.  Nevertheless we might need to handle here OVERFLOW?

We shouldn't, as above.

>>> @@ -1780,7 +1792,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);
>>>            else
>>>              /* We're in the middle of initializing it.  */
>>>              break;
>>> @@ -1864,7 +1877,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);
>>>         if (TREE_CODE (TREE_TYPE (field)) == INTEGER_TYPE
>>>            && TREE_CODE (value) == INTEGER_CST
>>>            && tree_fits_shwi_p (bitpos)
>>
>> Again, this shouldn't be necessary, either; the elements of the
>> CONSTRUCTOR should be fully evaluated already.
>
> Evaluated yes, but not necessarily folded to constant.  Maybe cp_fold_to_constant could be better choice here?

By evaluated I mean folded to constant.  We already called 
cxx_eval_constant_expression on all of the elements of the CONSTRUCTOR 
in cxx_eval_bare_aggregate.  If calling it again here does anything at 
all, something is broken.

>>>       case SIZEOF_EXPR:
>>> +      if (processing_template_decl
>>> +         && (!COMPLETE_TYPE_P (TREE_TYPE (t))
>>> +         || TREE_CODE (TYPE_SIZE (TREE_TYPE (t))) != INTEGER_CST))
>>> +       return t;
>>
>> Why is this necessary?
>
> We don't want to resolve SIZEOF_EXPR within template-declarations for incomplete types, of if its size isn't fixed.  Issue is that we otherwise get issues about expressions without existing type (as usual within template-declarations for some expressions).

Yes, but we shouldn't have gotten this far with a dependent sizeof; 
maybe_constant_value just returns if 
instantiation_dependent_expression_p is true.

>>> @@ -3391,8 +3431,23 @@ cxx_eval_constant_expression (const constexpr_ctx
>>> *ctx, tree t,
>>>       case CONVERT_EXPR:
>>>       case VIEW_CONVERT_EXPR:
>>>       case NOP_EXPR:
>>> +    case UNARY_PLUS_EXPR:
>>>         {
>>> +       enum tree_code tcode = TREE_CODE (t);
>>>          tree oldop = TREE_OPERAND (t, 0);
>>> +
>>> +       if (tcode == 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;
>>> +         }
>>>          tree op = cxx_eval_constant_expression (ctx, oldop,
>>
>> Why doesn't the call to cxx_eval_constant_expression at the bottom here
>> handle oldop having TREE_OVERFLOW set?
>
> I just handled the case that we see here a wrapping NOP_EXPR around an overflow.  As this isn't handled by cxx_eval_constant_expression.

How does it need to be handled?  A NOP_EXPR wrapped around an overflow 
is there to indicated that the expression is non-constant, and it can't 
be simplified any farther.

Please give an example of what was going wrong.

>>> @@ -565,6 +571,23 @@ cp_gimplify_expr (tree *expr_p, gimple_seq *pre_p,
>>> gimple_seq *post_p)
>>>
>>>     switch (code)
>>>       {
>>> +    case SIZEOF_EXPR:
>>> +      if (SIZEOF_EXPR_TYPE_P (*expr_p))
>>> +       *expr_p = cxx_sizeof_or_alignof_type (TREE_TYPE (TREE_OPERAND
>>> (*expr_p,
>>> +                                                                      0)),
>>> +                                             SIZEOF_EXPR, false);
>>> +      else if (TYPE_P (TREE_OPERAND (*expr_p, 0)))
>>> +       *expr_p = cxx_sizeof_or_alignof_type (TREE_OPERAND (*expr_p, 0),
>>> +                                             SIZEOF_EXPR, false);
>>> +      else
>>> +       *expr_p = cxx_sizeof_or_alignof_expr (TREE_OPERAND (*expr_p, 0),
>>> +                                             SIZEOF_EXPR, false);
>>> +      if (*expr_p == error_mark_node)
>>> +       *expr_p = size_one_node;
>>> +
>>> +      *expr_p = maybe_constant_value (*expr_p);
>>> +      ret = GS_OK;
>>> +      break;
>>
>> Why are these surviving until gimplification time?
>
> This might be still necessary. I will retest, when bootstrap works.  As we now added SIZEOF_EXPR folding to cp_fold, and if we catch all expressions a sizeof can occure, this shouldn't be necessary anymore.  AFAIR I saw here some issues about initialzation for global-variables, which weren't caught.

Hmm, I wonder why you would see issues with global initializers that 
aren't seen on trunk?  In any case, if the issue is with global 
initializers, they should be handled sooner, not here.

>>> +static tree
>>> +cp_fold (tree x, hash_map<tree, tree> *fold_hash)
>>
>> Looks like cp_fold still doesn't call maybe_constant_value.
>
> Yes, it should do this just in case of seen PTRMEM_CST.  Or shall we try to invoke it on each iteration within cp_fold.  Later might be a bit costy, isn't it?

I was thinking the latter; as I keep saying, we want to fold constexpr 
function calls at this point, too.

Yes, it might be too costly without some adjustments to cache 
intermediate values, or at least whether particular subexpressions are 
constant.

>>> +  case CALL_EXPR:
>>> +    r = fold (x);
>>> +    if (TREE_CODE (r) != CALL_EXPR)
>>> +      {
>>> +       x = cp_fold (r, fold_hash);
>>> +       break;
>>> +      }
>>> +    {
>>> +      int i, m = call_expr_nargs (x);
>>> +      for (i = 0; i < m; i++)
>>> +        {
>>> +         CALL_EXPR_ARG (x, i) = cp_fold (CALL_EXPR_ARG (x, i), fold_hash);
>>> +       }
>>> +    }
>>> +    r = fold (x);
>>> +    if (TREE_CODE (r) != CALL_EXPR)
>>> +      {
>>> +       x = cp_fold (r, fold_hash);
>>> +       break;
>>> +      }
>>> +    return org_x;
>>
>> Why return org_x here?  Don't we still want to add this to the hash table?
>
> No, as we just tried here to fold expression-arguments, so that later fold might be able to optimize.  So x is intermediate, and we don't want to cache it.
>
> But well, we might want to hash here org_x itself ...

Right, that's what I was thinking.

>> I'm also nervous about clobbering the args in the original CALL_EXPR.
>> In most of cp_fold you build a new expression rather than change the
>> operands of the original one.
>
> Well, this shouldn't be problematic IMP.  I just wanted to avoid to make use of a node-copy, which might be for a call-expression expensive.  But well, as we now have cp_try_fold_to_constant, this could lead indeed to folded call-expressions too early.  Before cp_fold was just used in the final folding before gimplification, where these modifications weren't problematic at all.
> So we might want to avoid such argument-conversion in context of cp_try_fold_to_constant at all?

You could use a temporary vector like in the TREE_VEC folding code, and 
only create a new CALL_EXPR if there's a change?

>>> @@ -608,9 +608,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);
>>
>> I still think that cp_fold_convert should always call fold_convert, and
>> callers that we don't want to fold should call convert instead, or
>> another function that folds only conversion of constants.  We had talked
>> about the name "fold_cst", but I think that name isn't very clear; would
>> it make sense to just have convert always fold conversions of constants?
>
> We could introduce that, but we still have the issues about some unary-operations on constants, too.  So we could do for any conversion afterwards a call to cp_try_fold_to_constant, which should reflect that pretty well, beside within template-declarations ...

I think cp_try_fold_to_constant would do too much folding.  I think we 
only want to immediately fold conversions and negation.

>>> @@ -1529,8 +1532,11 @@ build_expr_type_conversion (int desires, tree expr,
>>> bool complain)
>>>     tree basetype = TREE_TYPE (expr);
>>>     tree conv = NULL_TREE;
>>>     tree winner = NULL_TREE;
>>> +  /* Want to see if EXPR is a constant.  See below checks for null_node.
>>> */
>>> +  tree expr_folded = cp_try_fold_to_constant (expr);
>>>
>>> -  if (expr == null_node
>>> +  STRIP_NOPS (expr_folded);
>>> +  if (expr_folded == null_node
>>
>> Again, we shouldn't need to fold to check for null_node, it only occurs
>> when explicitly written.  Folding should never produce null_node unless
>> the argument was already null_node.
>
> Well, we need to do this for diagnostic messages AFAIR.  We want to see if expression folded gets a constant, so that diagnostics getting displayed right.

Again, null_node is special.  It indicates that the user typed "__null". 
  That's what we're checking for here.  Folding is both unnecessary and 
undesirable.

>>> @@ -1548,7 +1554,7 @@ build_expr_type_conversion (int desires, tree expr,
>>> bool complain)
>>>       switch (TREE_CODE (basetype))
>>>         {
>>>         case INTEGER_TYPE:
>>> -       if ((desires & WANT_NULL) && null_ptr_cst_p (expr))
>>> +       if ((desires & WANT_NULL) && null_ptr_cst_p (expr_folded))
>>
>> Again, 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.
>
> We need to avoid useless conversion, so we should reduce to simple constant-value ...

No.  Again, in C++11 only "0" or "0L" is a null pointer constant.   A 
more complex expression that folds to 0 is NOT a null pointer constant. 
  Folding is actively harmful here.

And again, in C++98 mode null_ptr_cst_p already folds, so doing it here 
is redundant.

Was I unclear?

>>> @@ -8496,16 +8467,18 @@ compute_array_index_type (tree name, tree size,
>>> tsubst_flags_t complain)
>>>         SET_TYPE_STRUCTURAL_EQUALITY (itype);
>>>         return itype;
>>>       }
>>> -
>>> +
>>> +  /* We need to do fully folding to determine if we have VLA, or not.  */
>>> +  tree size_constant = cp_try_fold_to_constant (size);
>>
>> Again, we already called maybe_constant_value.
>
> Sure, but maybe_constant_value still produces nops ...

If someone tries to create an array with a size that involves arithmetic 
overflow, that's undefined behavior and we should probably give an error 
rather than fold it away.

>>> @@ -13078,6 +13042,8 @@ build_enumerator (tree name, tree value, tree
>>> enumtype, tree attributes,
>>>     if (value)
>>>       STRIP_TYPE_NOPS (value);
>>>
>>> +  if (value)
>>> +    value = cp_try_fold_to_constant (value);
>>
>> Again, this is unnecessary because we call cxx_constant_value below.
>
> See nops, and other unary-operations we want to reduce here to real constant value ...

The cxx_constant_value call below will deal with them.

>>> @@ -13102,6 +13068,7 @@ build_enumerator (tree name, tree value, tree
>>> enumtype, tree attributes,
>>>            if (value != NULL_TREE)
>>>              {
>>>                value = cxx_constant_value (value);
>>> +             STRIP_NOPS (value);
>>
>> Again, 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?
>
> As for an enumerator-value we might have overflows, which are silently ignored.

They shouldn't be ignored.  C++ requires that the value be constant, and 
overflow makes it non-constant.

> I will recheck this what example we have for this when bootstrap is working again.
>
>>> @@ -6136,6 +6139,10 @@ push_to_top_level (void)
>>>     else
>>>       need_pop = false;
>>>
>>> +  if (scope_chain)
>>> +    fm = scope_chain->fold_map;
>>> +  else
>>> +    fm = NULL;
>>>     if (scope_chain && previous_class_level)
>>>       store_class_bindings (previous_class_level->class_shadowed,
>>>                            &s->old_bindings);
>>> @@ -6167,6 +6174,9 @@ push_to_top_level (void)
>>>     FOR_EACH_VEC_SAFE_ELT (s->old_bindings, i, sb)
>>>       IDENTIFIER_MARKED (sb->identifier) = 0;
>>>
>>> +  if (!fm)
>>> +    fm = new hash_map<tree, tree>;
>>
>> Why are these hunks so far apart?  I would expect them to be all together.
>
> Oh, nothing special.  I just wanted to have base initialization together with the other if initializes, and doing then later near assignment the optional allocation.
> I can change this, if you prefer.

Thanks, I would prefer that.

>>> @@ -6473,7 +6473,8 @@ cp_parser_array_notation (location_t loc, cp_parser
>>> *parser, tree *init_index,
>>>           2. ARRAY [ EXP : EXP ]
>>>           3. ARRAY [ EXP : EXP : EXP ]  */
>>>
>>> -      *init_index = cp_parser_expression (parser);
>>> +      *init_index = cp_parser_expression (parser);
>>> +      *init_index = cp_try_fold_to_constant (*init_index);
>>>         if (cp_lexer_peek_token (parser->lexer)->type != CPP_COLON)
>>>          {
>>>            /* This indicates that we have a normal array expression.  */
>>> @@ -6484,10 +6485,12 @@ cp_parser_array_notation (location_t loc, cp_parser
>>> *parser, tree *init_index,
>>>         /* Consume the ':'.  */
>>>         cp_lexer_consume_token (parser->lexer);
>>>         length_index = cp_parser_expression (parser);
>>> +      length_index = cp_try_fold_to_constant (length_index);
>>>         if (cp_lexer_peek_token (parser->lexer)->type == CPP_COLON)
>>>          {
>>>            cp_lexer_consume_token (parser->lexer);
>>>            stride = cp_parser_expression (parser);
>>> +         stride = cp_try_fold_to_constant (stride);
>>
>> Again, why fold here, rather than later when something really wants a
>> constant?  If that ever actually occurs?
>
> This code for cilk expects that these statements are folded to constants.  It doesn't make much difference to move those folders to later locations, as we have to fold them early anyway.

Yes, I think we should fold them in the cilk code that expects that.

>>> @@ -6575,6 +6578,13 @@ 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 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 = cp_try_fold_to_constant (index);
>>
>> Similarly, for offsetof the folding should happen closer to where it is
>> needed.
>>
>> Why is it needed for decltype, which is querying the type of an expression?
>>
>> For NEGATE_EXPR, we had talked about always folding a NEGATE of a
>> constant; this isn't the right place to do it.
>
> Same as above, we need in those cases (and for -1 too) the constant values early anyway.  So I saw it as more logical to have done this conversion as soon as possible after initialization.

I don't think this is as soon as possible; we can fold the NEGATE_EXPR 
immediately when we build it, at the end of cp_build_unary_op.

I still wonder why any folding is necessary for decltype.  When I ask 
why, I want to know *why*, not just have you tell me again that it's 
needed.  I don't think it is.

For offsetof, I wonder if it makes sense to extend fold_offsetof_1 to 
handle whatever additional folding is needed here.  If not, then fold in 
finish_offsetof, before calling fold_offsetof.

>>> @@ -8031,7 +8041,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);
>>
>> Huh?  build_c_cast just calls cp_build_c_cast.
>
> Yes, as we don't want to do cast-folding too early.

My point is that this change doesn't actually change anything; either 
way we're calling cp_build_c_cast, with the same arguments.

>>> @@ -9869,6 +9881,7 @@ cp_parser_label_for_labeled_statement (cp_parser*
>>> parser, tree attributes)
>>>          cp_lexer_consume_token (parser->lexer);
>>>          /* Parse the constant-expression.  */
>>>          expr = cp_parser_constant_expression (parser);
>>> +       expr = cp_try_fold_to_constant (expr);
>>>          if (check_for_bare_parameter_packs (expr))
>>>            expr = error_mark_node;
>>>
>>> @@ -9878,6 +9891,7 @@ cp_parser_label_for_labeled_statement (cp_parser*
>>> parser, tree attributes)
>>>              /* Consume the `...' token.  */
>>>              cp_lexer_consume_token (parser->lexer);
>>>              expr_hi = cp_parser_constant_expression (parser);
>>> +           expr_hi = cp_try_fold_to_constant (expr_hi);
>>
>> Again, this seems redundant with the call to cxx_constant_value in
>> case_conversion.
>
> Well, but we want to do here check_for_bare_parameter_packs directly after that, and so we need folded value early.

Why would check_for_bare_parameter_packs need a folded value?

>>> @@ -16115,6 +16133,7 @@ cp_parser_enumerator_definition (cp_parser* parser,
>>> tree type)
>>>         cp_lexer_consume_token (parser->lexer);
>>>         /* Parse the value.  */
>>>         value = cp_parser_constant_expression (parser);
>>> +      value = cp_try_fold_to_constant (value);
>>
>> Again, this is redundant with the code in build_enumerator.
>
> Hmm, don't see here redundancy.  I just see that we need folded-value for leter call of check_for_bare_parameter_packs.  But might not see the obvious

As above, I don't see any connection between 
check_for_bare_parameter_packs and folding.

>>> @@ -19354,12 +19374,10 @@ cp_parser_initializer_clause (cp_parser* parser,
>>> bool* non_constant_p)
>>>     /* If it is not a `{', then we are looking at an
>>>        assignment-expression.  */
>>>     if (cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE))
>>> -    {
>>> -      initializer
>>> -       = cp_parser_constant_expression (parser,
>>> -                                       /*allow_non_constant_p=*/true,
>>> -                                       non_constant_p);
>>> -    }
>>> +    initializer
>>> +      = cp_parser_constant_expression (parser,
>>> +                                     /*allow_non_constant_p=*/true,
>>> +                                     non_constant_p);
>>
>> Let's not reformat unrelated code in a big project like this.
>
> Well, but it violates coding-rules.  anyway agreed ;)

If you want to apply that fix to the trunk, go ahead, but it's 
distracting when looking at the branch.  :)

>>> @@ -20955,9 +20973,11 @@ cp_parser_member_declaration (cp_parser* parser)
>>>
>>>                /* Consume the `:' token.  */
>>>                cp_lexer_consume_token (parser->lexer);
>>> +
>>>                /* Get the width of the bitfield.  */
>>>                width
>>>                  = cp_parser_constant_expression (parser);
>>> +             width = maybe_constant_value (width);
>>
>> Again, this seems redundant with the call to cxx_constant_value in
>> check_bitfield_decl.
>
> Ah, this escaped.  This should be a cp_try_fold_to_constant.

It should be removed entirely, because it's redundant.

>> Again, it seems like you added maybe_constant_value or
>> cp_try_fold_to_constant 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.
>
> Actually it should be cp_try_fold_to_constant, as at those places we need to deal anyway with constant-values.  At some please we use those values already for some diagnostics, so I thoght it is more consistent to do this folding to constant directly after parsing it.  To delay that into builder-routines is IMO just less clear, and could lead to double-doing foldings.  Additionally the chance to conflict here with shared parts with C is much less.
>
> Anyway, if you prefer, we can do this in builder-routines, and remove at places constants aren't needed directly after parsing it those calls.

I want to delay it to:

1) the places where we actually care about constant values, all of which 
already call maybe_constant_value or cxx_constant_value, so they 
shouldn't need much change; and
2) the places where we want a simplified expression for warnings, where 
we should call cp_fully_fold.

Folding in the parser is wrong, most of all because template 
substitution doesn't go through the parser.

>>>   finish_unary_op_expr (location_t loc, enum tree_code code, tree expr,
>>>                        tsubst_flags_t complain)
>>>   {
>>> +  tree expr_ovl = expr;
>>>     tree result = build_x_unary_op (loc, code, expr, complain);
>>> +  tree result_ovl = result;
>>> +
>>> +  STRIP_NOPS (expr_ovl);
>>> +  switch (code)
>>> +    {
>>> +    case ABS_EXPR:
>>> +    case NEGATE_EXPR:
>>> +      if (TREE_CODE (expr) == INTEGER_CST
>>> +         || TREE_CODE (expr) == REAL_CST
>>> +         || TREE_CODE (expr) == VECTOR_CST
>>> +         || TREE_CODE (expr) == FIXED_CST
>>> +         || TREE_CODE (expr) == COMPLEX_CST)
>>> +      result_ovl = fold (result);
>>> +      break;
>>> +    default:
>>> +      break;
>>> +    }
>>
>> Not cp_fully_fold?
>
> Hmm, yeah,  I changed the switch to a call to cp_try_fold_to_constant, which should be pretty exactly what we are looking here for diagnostics.

For diagnostics we want cp_fully_fold, so we get a simplified expression 
even if it isn't constant.

>>> @@ -6301,8 +6321,9 @@ handle_omp_for_class_iterator (int i, location_t
>>> locus, tree declv, tree initv,
>>>                                      tf_warning_or_error);
>>>         if (error_operand_p (iter_incr))
>>>          return true;
>>> -      else if (TREE_CODE (incr) == PREINCREMENT_EXPR
>>> -              || TREE_CODE (incr) == POSTINCREMENT_EXPR)
>>> +      iter_incr = fold (iter_incr);
>>> +      if (TREE_CODE (incr) == PREINCREMENT_EXPR
>>> +         || TREE_CODE (incr) == POSTINCREMENT_EXPR)
>>> @@ -6357,6 +6376,7 @@ handle_omp_for_class_iterator (int i, location_t
>>> locus, tree declv, tree initv,
>>>                                                   tf_warning_or_error);
>>>                    if (error_operand_p (iter_incr))
>>>                      return true;
>>> +                 iter_incr = fold (iter_incr);
>>>                    iter_incr = build_x_modify_expr (EXPR_LOCATION (rhs),
>>>                                                     iter, NOP_EXPR,
>>>                                                     iter_incr,
>>> @@ -6364,6 +6384,7 @@ handle_omp_for_class_iterator (int i, location_t
>>> locus, tree declv, tree initv,
>>>                    if (error_operand_p (iter_incr))
>>>                      return true;
>>>                    incr = TREE_OPERAND (rhs, 0);
>>> +                 incr = fold (incr);
>>
>> Why are these folds needed?

Still wondering.

>>> @@ -441,7 +441,7 @@ build_aggr_init_expr (tree type, tree init)
>>>     else if (TREE_CODE (init) == AGGR_INIT_EXPR)
>>>       fn = AGGR_INIT_EXPR_FN (init);
>>>     else
>>> -    return convert (type, init);
>>> +    return fold (convert (type, init));
>>
>> Why fold here?
>
> We had this already in prior thread.  fold (convert ()) != fold_convert () for C++.  The fold is just there to make sure we fold away useless casts.

But why here?  Can't we fold away useless casts earlier (in convert) or 
later (when we care about having a simplified expression)?

>>> @@ -3664,6 +3660,10 @@ convert_arguments (tree typelist, vec<tree, va_gc>
>>> **values, tree fndecl,
>>>            && (type == 0 || TREE_CODE (type) != REFERENCE_TYPE))
>>>          val = TREE_OPERAND (val, 0);
>>>
>>> +      /* 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);
>>
>> Why?
>
> As builtin-handlers are expecting to see constant values.

Doesn't cp_fold fold the arguments before trying to fold the calls?

Hmm, I guess build_cxx_call calls check_builtin_function_arguments, 
which expects constants for some builtins.  Maybe a C++ wrapper for that 
function could fold the arguments before passing them along.

> Otherwise the produce either ICE, or other funny things.  We could call here instead also the cp_try_fold... function.
> Btw some lines above there is a comment about NOP_EXPR being extended to indicate that a value isn't lvalue.  Not sure if this NOP_EXPR strip is still necessary, as we don't call anymore directly into build_c_cast ().  I need to check if cp-version does the same.
>
>>> @@ -4941,7 +4938,7 @@ cp_build_binary_op (location_t location,
>>>               from being kept in a register.
>>>               Instead, make copies of the our local variables and
>>>               pass the copies by reference, then copy them back afterward.
>>>               */
>>> -         tree xop0 = op0, xop1 = op1, xresult_type = result_type;
>>> +         tree xop0 = fold (op0), xop1 = fold (op1), xresult_type =
>>> result_type;
>>
>> Again, this seems wrong.  In fact, the whole short_compare business
>> seems like the sort of early folding we want to do away with.
>>
>>> @@ -5026,18 +5023,21 @@ cp_build_binary_op (location_t location,
>>>       }
>>>
>>>     result = build2 (resultcode, build_type, op0, op1);
>>> -  result = fold_if_not_in_template (result);
>>>     if (final_type != 0)
>>>       result = cp_convert (final_type, result, complain);
>>> -
>>> -  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);
>>
>> Don't you want to use cp_fully_fold here?

?

>>> @@ -7249,7 +7249,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);
>>
>> Why didn't delayed folding canonicalize this so that the decl is in op0?
>
> Delay folding doesn't canonicalize this.

Why not?  Doesn't it fold all expressions?

> Actually we don't want to touch here anything in parsered tree.  We could do this in generalization-pass before gimplification.  Seems to be something we don't catch for now, which makes me wonder a bit.
>
>>> @@ -508,7 +508,9 @@ extract_omp_for_data (gomp_for *for_stmt, struct
>>> omp_for_data *fd,
>>>            gcc_assert (gimple_omp_for_kind (for_stmt)
>>>                        == GF_OMP_FOR_KIND_CILKSIMD
>>>                        || (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));
>>
>> This still seems like a red flag; how is delayed folding changing the
>> OMP for kind?
>
> It doesn't.  The issue is that some canonical operations of fold aren't happening anymore on which omp depends.

That seems like a problem.

>>> @@ -1947,6 +1947,8 @@ build_complex (tree type, tree real, tree imag)
>>>   {
>>>     tree t = make_node (COMPLEX_CST);
>>>
>>> +  real = fold (real);
>>> +  imag = fold (imag);
>>
>> I still think this is wrong.  The arguments should be sufficiently folded.
>
> As we don't fold unary-operators on constants, we need to fold it at some place.  AFAICS is the C++ FE not calling directly build_complex.  So this place was the easiest way to avoid issues with things like '-' '1' etc.

Is this because of the
>       value = build_complex (NULL_TREE, convert (const_type,
>                                                  integer_zero_node), value);
in interpret_float?  I think "convert" definitely needs to do some 
folding, since it's called from middle-end code that expects that.

>>> @@ -5080,6 +5081,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);
>>
>> Likewise.
>
> As soon as we can be sure that values getting fully_folded, or at least folded for constants, we should be able to remove this.

Yep, they need to be folded before we get here.

It looks like your latest checkin added more redundant folding:

> @@ -3311,6 +3311,9 @@ finish_case_label (location_t loc, tree low_value, tree hi
> gh_value)
>    low_value = case_conversion (type, low_value);
>    high_value = case_conversion (type, high_value);
>
> +  low_value = cp_fully_fold (low_value);
> +  high_value = cp_fully_fold (high_value);

Again, case_conversion should have already folded constants.

> @@ -5776,6 +5776,8 @@ convert_nontype_argument (tree type, tree expr, tsubst_flags_t complain)
>  {
>    tree expr_type;
>
> +  expr = cp_try_fold_to_constant (expr);
> +
>    /* Detect immediately string literals as invalid non-type argument.
>       This special-case is not needed for correctness (we would easily
>       catch this later), but only to provide better diagnostic for this
> @@ -5852,6 +5854,7 @@ convert_nontype_argument (tree type, tree expr, tsubst_flags_t complain)
>        else if (TYPE_PTR_OR_PTRMEM_P (type))
>         {
>           tree folded = maybe_constant_value (expr);
> +         folded = cp_try_fold_to_constant (expr);

And here, convert_nontype_argument already uses 
maybe_constant_value/cxx_constant_value for folding constants.

Jason