2015-07-30 0:56 GMT+02:00 Kai Tietz <ktiet...@googlemail.com>:
> 2015-07-29 19:48 GMT+02:00 Jason Merrill <ja...@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
