Re: C++ delayed folding branch review

[Kai Tietz <ktietz@redhat.com>]

----- Ursprüngliche Mail -----
> On 07/30/2015 05:00 PM, Kai Tietz wrote:
> > 2015-07-30 18:52 GMT+02:00 Jason Merrill <jason@redhat.com>:
> >> On 07/29/2015 06:56 PM, Kai Tietz wrote:
> >>>>>>>>>>> @@ -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?
> >>
> >>
> >> We call cxx_constant_value on bit-field widths in check_bitfield_decl.
> >
> > Hmm, ok.  But I don't see that this function gets called in context of
> > grokbitfield, after we set DECL_INITIAL.
> 
> Nope, it's called later on as part of finish_struct.
> 

Ok, adjusted.

> > By removing this folding here, we get new failures in
> > g++.dg/warn/overflow-warn-1.C testcase:
> > New errors are at lin 32 that 'bif-foeld 's::<anonymous>' width not an
> > integer constant'
> > and at same line ''(1 / 0) is not a constant expression'.  Those
> > message don't look wrong.
> >
> > The testcase next to this 'overflow-warn-3.C and overflow-warn-4.C'
> > failing in the same manner for (1 / 0) case.  But there are no other
> > regressions in g++.dg & libstdc++
> >
> > Shall I extend the testcases for this message?
> 
> Please.

Done.
 
> >>>>>>>>>>> @@ -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.
> >>
> >>
> >> So can we remove the fold in build_complex now?
> 

Yes. Done.

> 
> >>>>>>>> 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?
> >>
> >>
> >> In the context of this work, if they are introducing problematic NOPs.
> >
> > Ok, I will take a closer look to convert () usage in c-family/.  By
> > quick looking this seems to be the only place for now we needed to
> > change.
> >
> >>>>>>>>>>> @@ -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'.
> >>
> >>
> >> That indicates a folding problem earlier on, that will cause some
> >> initialization that should be performed at compile time to happen at run
> >> time instead.
> >>
> >> Please investigate the bootstrap issue further.
> >
> > Yes, I do. I assume it is related to 'store_init_value'.  For cases
> > decl_maybe_constant_var_p() is true, or decl is a static, we are
> > calling maybe_constant_init on the value, but for other cases we don't
> > simplify value. (Btw this might be related to the
> > STRIP_NOPS-requirement in 'reduced_constant_expression_p').  So by
> > adding the following hunk it seems to work (still need to verify)
> >
> > Index: typeck2.c
> > ===================================================================
> > --- typeck2.c   (Revision 226401)
> > +++ typeck2.c   (Arbeitskopie)
> > @@ -833,6 +833,8 @@ store_init_value (tree decl, tree init, vec<tree,
> >         DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (decl) = const_init;
> >         TREE_CONSTANT (decl) = const_init && decl_maybe_constant_var_p
> >         (decl);
> >       }
> > +  else
> > +    value = fold_simple (value);
> >
> >     if (cxx_dialect >= cxx14 && CLASS_TYPE_P (strip_array_types (type)))
> >       /* Handle aggregate NSDMI in non-constant initializers, too.  */
> 
> I guess we want to extend the code for handling statics and constants to
> also handle the case where the initializer is a CONSTRUCTOR.  And also
> fold individual elements in split_nonconstant_init.

Yes, I added a general full folding to store_init_value, and to split_nonconstant_init a folding to CONSTRUCTOR handling for RECORD_TYPES, etc.  By this I was able to remove the fold from varasm.c's output-function, too.
The invocation in split_nonconstant_init might not be necessary, but there are other callers then store_init_value.  So for them folding in those cases might be still benifitial.


The "STRIP_NOPS-requirement in 'reduced_constant_expression_p'" I could remove, but for one case in constexpr.  Without folding we don't do type-sinking/raising.  So binary/unary operations might be containing cast, which were in the past unexpected.  On verify_constant we check by reduced_constant_expression_p, if value is a constant.  We don't handle here, that NOP_EXPRs are something we want to look through here, as it doesn't change anything if this is a constant, or not.
So I suggest following patch, so we are able to remove the STRIP_NOPS from reduced_constant_expression_p.

--- constexpr.c	(revision 226452)
+++ constexpr.c	(working copy)
@@ -1441,8 +1441,6 @@ cxx_eval_call_expression (const constexpr_ctx *ctx
 bool
 reduced_constant_expression_p (tree t)
 {
-  /* Make sure we remove useless initial NOP_EXPRs.  */
-  STRIP_NOPS (t);
   switch (TREE_CODE (t))
     {
     case PTRMEM_CST:
@@ -1476,7 +1474,10 @@ static bool
 verify_constant (tree t, bool allow_non_constant, bool *non_constant_p,
 		 bool *overflow_p)
 {
-  if (!*non_constant_p && !reduced_constant_expression_p (t))
+  tree rde = t;
+
+  STRIP_NOPS (rde);
+  if (!*non_constant_p && !reduced_constant_expression_p (rde))
     {
       if (!allow_non_constant)
 	error ("%q+E is not a constant expression", t);


Regards,
Kai
 
> Jason
> 
>