From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 126842 invoked by alias); 29 Jul 2015 22:56:45 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 126826 invoked by uid 89); 29 Jul 2015 22:56:44 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-wi0-f175.google.com Received: from mail-wi0-f175.google.com (HELO mail-wi0-f175.google.com) (209.85.212.175) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Wed, 29 Jul 2015 22:56:40 +0000 Received: by wicmv11 with SMTP id mv11so236778595wic.0 for ; Wed, 29 Jul 2015 15:56:37 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.194.192.166 with SMTP id hh6mr78405908wjc.127.1438210597303; Wed, 29 Jul 2015 15:56:37 -0700 (PDT) Received: by 10.27.176.72 with HTTP; Wed, 29 Jul 2015 15:56:37 -0700 (PDT) In-Reply-To: <55B911DD.30105@redhat.com> References: <557A5214.7060106@redhat.com> <1424811417.1214725.1434125493982.JavaMail.zimbra@redhat.com> <557BAE5A.7030309@redhat.com> <55B661A1.6090308@redhat.com> <55B911DD.30105@redhat.com> Date: Wed, 29 Jul 2015 23:03:00 -0000 Message-ID: Subject: Re: C++ delayed folding branch review From: Kai Tietz To: Jason Merrill Cc: Kai Tietz , gcc-patches List Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2015-07/txt/msg02525.txt.bz2 2015-07-29 19:48 GMT+02:00 Jason Merrill : > On 07/28/2015 04:10 PM, Kai Tietz wrote: >> >> 2015-07-28 1:14 GMT+02:00 Kai Tietz : >> >>> 2015-07-27 18:51 GMT+02:00 Jason Merrill : >>>> >>>> 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>>>>>>> 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