Re: C++ delayed folding branch review

[Kai Tietz <ktietz70@googlemail.com>]

2015-07-30 0:56 GMT+02:00 Kai Tietz <ktietz70@googlemail.com>:
> 2015-07-29 19:48 GMT+02:00 Jason Merrill <jason@redhat.com>:
>> On 07/28/2015 04:10 PM, Kai Tietz wrote:
> The change to adjust_temp_type seems to be no more necessary (just
> doing tests on it).

Yes, committed it.

>>
>>>>>>>>> @@ -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.
>>
>> ^
>
> I did some regression-testing on it.  This looks to me like something
> I missed to cleanup.  Most changes within constexpr-code aren't
> necessary anymore.  But looking on that, I think I papered over some
> issues I had about double-reporting of non-constant expression on
> overflows.

Committed change to branch for removing this.

>>>>>>>>> @@ -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.
>>
>
> They don't survice in function-context, but outside they might.  On
> trunk we never will see an sizeof-expression in such case as they got
> folded-away much earlier.
>
> I will try an bootstrap with disabling it.  In ME we don't produce
> sizeof-expressions anymore, so we don't need to think about
> re-gimplifiying some AST AFAICS.

Tested.  We seems not to need the handle of SIZEOF_EXPR in gimplifier
anymore.  so removed hunk.

>>
>>>>>>>>> @@ -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.
>>
>
> So, let us remove it ...  I expect issues about casts on integers,
> which are reasoned due implicit assignments, expr won't be null_node.

I was wrong about this.  I removed the folding from
build_expr_type_conversion routine.

>>
>>>>>>>>> @@ -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?
>>
>>
>> ^

See comment above. Cleaned up.

>>>>>>>>> @@ -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.
>>>>>
>>>>>
>>>>>
>>>>> Likewise for grokbitfield.
>>>>
>>>>
>>>> Hmm, AFAIR we don't call cxx_constant_value in all code-paths.  But I
>>>> will look into it, and come back to you on it.
>>>
>>>
>>> I am still on it ...  first did the other points
>>
>>
>> Looks like this hasn't changed.
>
> Yes, for grokbitfield current version uses fold_simple for witdth.  So
> just expressions based on constants getting reduced to short form.  In
> grokbitfield I don't see invocation of cxx_constant_value.  So how can
> we be sure that width is reduced to integer-cst?
>
> For build_enumerator the call is indeed superflous.
I modified build_enumerator not to fold additionally.


>>>>>>>>> @@ -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)?
>>
>>
>> ^

I removed that fold.

>>>>>>>>> @@ -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.
>>>>>
>>>>>
>>>>> I would think this should be maybe_constant_value then.
>>>>
>>>>
>>>> Why?  At the end we resolve normal-builtin via 'fold_call_expr'.  Of
>>>> course we can invoke here maybe_constant_value, but it would end up in
>>>> the same folding of a builtin-expression. So calling here directly
>>>> 'fold' just short-cuts this.
>>>
>>>
>>> ?
>>
>>
>> Wait.  Why are we folding here, again?  Which builtins need to have constant
>> values here, before late folding?
>
> Well, I would have assumed here first that on builitin-functions, we
> need to fold arguments (at least the constant values), as
> builtin-folder-routines are depending on seeing them (eg.
> builtin_expect, etc).  But by looking on code, I would assume that
> this method doesn't do this ...
> I am confused.  So I would assume that this fold in convert_arguments
> for builtin-normal functions seems not to be necessary.  We should
> handle this at other places already (and better).

I removed this hunk, and retested.  It isn't necessary anymore, so I removed it.

>>>>>> @@ -867,7 +867,7 @@ expand_subword_shift (machine_mode op1_mode, optab
>>>>>> binoptab,
>>>>>>           are truncated to the mode size.  */
>>>>>>         carries = expand_binop (word_mode, reverse_unsigned_shift,
>>>>>>                                outof_input, const1_rtx, 0, unsignedp,
>>>>>> methods);
>>>>>> -      if (shift_mask == BITS_PER_WORD - 1)
>>>>>> +      if (shift_mask == (unsigned HOST_WIDE_INT) (BITS_PER_WORD - 1))
>>>>>
>>>>>
>>>>>
>>>>> These should still be unnecessary.
>
> No more.
>
>> Looks like this is still there.
>
> Right, didn't noticed that I haven't caught them too, while cleaning
> those no longer required signed/unsigned cast modifications for
> bootstrap.

Removed it.

>>>>>>>>> @@ -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.
>>
>
> I didn't come to remove this line for testing.  As we fold now for
> initializers more early, and cp_fold supports constructors, it could
> be that we don't need this anymore.  It is on my pile.

That fold is still required.  By removing it, I saw boostrap issue due
'invalid initializer'.

>>>>>>> @@ -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.
>>
>
> Yes, folding is here superflous.  I will remove it.

Removed it from branch.

Kai