From: Kai Tietz <ktietz70@googlemail.com>
To: Jason Merrill <jason@redhat.com>
Cc: Kai Tietz <ktietz@redhat.com>,
gcc-patches List <gcc-patches@gcc.gnu.org>
Subject: Re: C++ delayed folding branch review
Date: Wed, 29 Jul 2015 23:03:00 -0000 [thread overview]
Message-ID: <CAEwic4Z-MgOwdyY_GTP+hGrK6qHgRoys8d4Tj_kHMri7oGuqHQ@mail.gmail.com> (raw)
In-Reply-To: <55B911DD.30105@redhat.com>
2015-07-29 19:48 GMT+02:00 Jason Merrill <jason@redhat.com>:
> On 07/28/2015 04:10 PM, Kai Tietz wrote:
>>
>> 2015-07-28 1:14 GMT+02:00 Kai Tietz <ktietz70@googlemail.com>:
>>
>>> 2015-07-27 18:51 GMT+02:00 Jason Merrill <jason@redhat.com>:
>>>>
>>>> I've trimmed this to the previously mentioned issues that still need to
>>>> be
>>>> addressed; I'll do another full review after these are dealt with.
>>>
>>>
>>> Thanks for doing this summary of missing parts of prior review.
>>>
>>>> On 06/13/2015 12:15 AM, Jason Merrill wrote:
>>>>>
>>>>>
>>>>> On 06/12/2015 12:11 PM, Kai Tietz wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> @@ -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);
>
> ^
>
Checked, and removing those STRIP_NOPS cause regressions about
vector-casts. At least the STRIP_NOPS in
reduced_constant_expression_p seems to be required. See as example
g++.dg/ext/vector20.C as testcase.
It sees that '(vec)(const __vector(2) long int){3l, 4l}' is not a
constant expression.
The change to adjust_temp_type seems to be no more necessary (just
doing tests on it).
>>>>>>>> @@ -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.
Replaced the x = get_nth_callarg (t,i); by a gcc_unreachable ();,
just to be sure we hit issue, if occures.
>>>>>>
>>>>>>
>>>>>> 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.
>
>
> ^
>
>>>>>>>> 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?
The issue is that by delayed-folding we don't fold sizeof-expressions
until we do the folding after genericize-pass. So those expressions
remain, and we can run in template on sizeof-operators on incomplete
types, if we invoke here variants of the constexpr-code. So this
pattern simply verifies that the sizeof-operand can be determined. We
could simply avoid resolving sizeof-operators in template-decl at all.
But my idea here was to try to resolve them, if the type of the
operand is already complete (and has an constant size).
>>>>>>
>>>>>>
>>>>>> 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.
>
> ^
Well, but we could come here by other routine then
maybe_constant_value. For example cxx_constnat_value doesn't do checks
here.
>
>>>>>>>> @@ -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.
>>>>>>>> @@ -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.
>
>>>>>>>> @@ -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.
>
>>>>>>>> @@ -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. I will remove folding, and check.
>>>>>>>> @@ -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.
>
>
> ^
If we need to do some reduction to constant value here, as expr might
be actually a constant, which isn't folded here. Eg something like:
struct {
char abc[sizeof (int) * 8];
};
Due delayed folding array index isn't necessarily reduced here. So we
need to perform at least constant value folding for diagnostics, as we
do right now.
>>>>>>>> @@ -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.
>>>>>>>> @@ -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.
>>>>
>>>>
>>>>
>>>> I see that this is now an unconditional fold_simple, but I still don't
>>>> understand why it needs to be folded here, in the parser.
>>>
>>>
>>> The point to fold the 'value' here is for cases
>>> 'processing_template_decl' isn't false. We could move it to the
>>> else-case of the 'if (! processing_template_decl)' line for being more
>>> explicit?
>>
>>
>> Well, on looking here in more detail, we might don't that that initial
>> folding here. As for processing_template_decl fold_simple (and
>> cp_fully_fold) doesn't do much.
>
>
> Looks like the fold is still there.
Yes, but a fold_simple one just working on constant values. It
doesn't fold expressions like 'a == a' to a constant. I extended
comment in current version on branch. Additionally it invokes now the
fold_simple always. We want to reduce index, if possible, for
diagnostics in code in c-family/c-common.c for array-bounds, for types
(they need to be fully folded), and to be sure we simplify basic
operations on constant-values.
>>>>>> 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 fold_simple.
>>>>
>>>>
>>>>> Folding in the parser is wrong, most of all because template
>>>>> substitution doesn't go through the parser.
>>>>
>>>>
>>>> There are still several folds in cp_parser_omp_* that should move later.
>>>
>>>
>>> In 'cp_parser_omp_var_list_no_open' we need to fold 'length' can
>>> 'low_bound' as those values getting checked some lines below (see
>>> lines 27936, 27944).
>
>
> OK, but this seems like an typical case of needing to fold for diagnostics;
> usually in those cases you use the folded value for the diagnostics and then
> keep using the unfolded expression elsewhere.
Right.
>>> In 'cp_parser_cilk_grainsize' we fold 2nd argument of
>>> 'cp_paser_cild_for' by 'fold_simple'. Not sure if it is worth to move
>>> operand-folding into cp_parser_cilk_for itself, as we have here just
>>> two users of 'cp_parser_cilk_for'.
>>> One time we pass 'integer_zero_node' as this argument, and the other
>>> time a binary-expression, which might be constant value.
>>> But sure we can move it into 'cp_parser_cilk_grainsize'.if you prefer?
>>
>>
>> ?
>
>
> Why does the fold need to be in the parser?
Well, if we hit it during our tree-walk in cp_fold_r, then we don't
need to fold it here. I will check, if this is really necessary.
>>>>>>>> @@ -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.
>>>>
>>>>
>>>> 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).
>>>>>>>> @@ -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?
>>>>
>>>>
>>>> ?
>>>
>>>
>>> It fold them lately. I will recheck this code-change. It might be no
>>> longer required due recent changes to omp-folding. It could be that
>>> original pattern didn't applied here anymore, and therefore statement
>>> didn't been transformed into its canonical form. Bit I assume this
>>> could be resolved.
>
>
> ?
This hunk is necessary as we don't use cannonical-form produced by
shorten_compare anymore. Therefore special operand can occure on
right-hand side too.
>>>>> @@ -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.
>>>>>>>> @@ -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);
>>>
>>>
>>> Might be. This should be indeed a 'fold_convert', isn't it?
>
>
> Yes.
Applied modification to it.
>>>>> in interpret_float? I think "convert" definitely needs to do some
>>>>> folding, since it's called from middle-end code that expects that.
>>>>
>>>>
>>>> I remember talking about "convert" doing some folding (and cp_convert
>>>> not)
>>>> in our 1:1 last week.
>>>
>>>
>>> Can't remember that. I know that we were talking about the difference
>>> of convert and fold_convert. convert can be used on C++ specifics,
>>> but fold_convert is something shared with ME.
>
>
> convert is called from the ME, which sometimes expects folding.
>
>>> So first 'fold_convert'
>>> isn't the same as 'fold (convert ())'.
>>> I don't find places we invoke convert () in ME. We have some calls in
>>> convert.c (see convert_to_integer, convert_to_integer_nofold, and
>>> convert_to_real), which all used in AST only AFAICS.
>
>
> I was thinking of convert.c and fold-const.c to be part of the ME, since
> they are language-independent. But I guess other people think of the ME
> starting with gimple.
>
> And it looks like the only language-independent uses of convert are in
> c-family; I guess many of them should change to fold_convert.
Hmm, in context of this work? Or is this more a general point about future work?
>>> I remember that we were talking about adding a standard-folding to
>>> convert for operations on constant-values (as we do for
>>> convert_to_integer). Do you mean this?
>
>
> Yes. But it seems that isn't necessary.
>
>>>>>>>> @@ -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.
>>>>>> @@ -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.
>
>>>>>> @@ -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);
>>>>>> +
>>>>>
>>>>>
>>>>> And here, convert_nontype_argument already uses
>>>>> maybe_constant_value/cxx_constant_value for folding constants.
>
Yes, this invocation looks useless too. I think I introduced it for
the STRING_CST check below, but AFAICS we should assume it as
unnecessary. I will change it and do regression-testing.
>
> Jason
By recent changes we seem to hit for c++ some additional regression.
They are related to negate-shifts for c++11. We are hitting now the
check within cxx_constant_value. The cxx_eval_check_shift_p sees now
that left-hand operand is negative and produces two new errors for
following tests: c-c++-common Wshift-negateive-value-*.c cases.
So we will need adjust those cases, or invoke within this
eval-function instead maybe_constant_value to avoid that ?
Kai
next prev parent reply other threads:[~2015-07-29 22:56 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-12 5:41 Jason Merrill
2015-06-12 16:17 ` Kai Tietz
2015-06-13 7:58 ` Jason Merrill
2015-07-27 19:01 ` Jason Merrill
2015-07-28 2:40 ` Kai Tietz
2015-07-28 20:35 ` Kai Tietz
2015-07-29 18:48 ` Jason Merrill
2015-07-29 23:03 ` Kai Tietz [this message]
2015-07-30 14:40 ` Kai Tietz
2015-07-30 18:41 ` Jason Merrill
2015-07-30 21:33 ` Kai Tietz
2015-07-31 0:43 ` Jason Merrill
2015-07-31 7:08 ` Jeff Law
2015-07-31 23:00 ` Kai Tietz
2015-08-03 3:49 ` Jason Merrill
2015-08-03 9:42 ` Kai Tietz
2015-08-03 15:39 ` Jason Merrill
2015-08-24 7:20 ` Kai Tietz
2015-08-27 2:57 ` Jason Merrill
2015-08-27 10:54 ` Kai Tietz
2015-08-27 13:35 ` Jason Merrill
2015-08-27 13:44 ` Kai Tietz
2015-08-27 18:15 ` Kai Tietz
2015-08-28 3:03 ` Jason Merrill
2015-08-28 7:43 ` Kai Tietz
2015-08-28 11:18 ` Kai Tietz
2015-08-28 2:12 ` Jason Merrill
2015-07-31 4:00 ` Jeff Law
2015-07-31 16:26 ` Jason Merrill
2015-07-31 16:43 ` Kai Tietz
2015-07-31 16:52 ` Jakub Jelinek
2015-07-31 16:53 ` Jason Merrill
2015-07-31 21:31 ` Kai Tietz
-- strict thread matches above, loose matches on Subject: below --
2015-04-24 4:23 Jason Merrill
2015-04-24 13:46 ` Kai Tietz
2015-04-24 18:25 ` Jason Merrill
2015-04-28 12:06 ` Kai Tietz
2015-04-28 13:57 ` Jason Merrill
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAEwic4Z-MgOwdyY_GTP+hGrK6qHgRoys8d4Tj_kHMri7oGuqHQ@mail.gmail.com \
--to=ktietz70@googlemail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jason@redhat.com \
--cc=ktietz@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).