Re: C++ delayed folding branch review

[Jason Merrill <jason at redhat dot com>]

On 04/28/2015 08:06 AM, Kai Tietz wrote:
> 2015-04-24 20:25 GMT+02:00 Jason Merrill <jason@redhat.com>:
> > So for warning folding, I think
> > the caching approach we were talking about in the call today is the way to
> > go.
>
> Ok.  Just a question about where to hook up the hash-variable.  What
> would be a good place to create (well this we could do lazy too) it,
> and of more importance where to destroy the hash? We could destroy it
> after genericize-pass?

The important thing is to avoid keeping exprs in the hash table that 
have already been ggc_collected.  So we want to destroy the hash table 
at least after every top-level declaration, but destroying it more 
frequently would be fine too if memory bloat seems like an issue.

> > Certainly a constant value from maybe_constant_value should not have a
> > useless type conversion wrapped around it, as that makes it appear
> > non-constant.
>
> Well, beside an overflow was seen.

Right.

> The interesting point is here to
> find the proper place to do for constructor-elements proper folding,
> or at least constant-value folding.

cxx_eval_bare_aggregate should already be doing constant-value folding 
of constructor elements.

> > I think that's exactly what I was saying above: "we can't just use
> > maybe_constant_value because that only folds C++ constant-expressions, and
> > we want to fold more things than that."
>
> maybe_constant_value folds constants, too.  That was actually the
> reason to use it.  Nevertheless I admit that we could call instead a
> ..fully_fold here too.

Call it where?

> One point I see here about creating a function using
> maybe_constant_value + cp_fold is that maybe_constant_value is
> something we can call safely in all cases.

Right, we would still want the current maybe_constant_value for the few 
situations where constant expressions affect language semantics but are 
not required, such as initializers and array bounds.

I'm suggesting that we should add maybe_constant_value to cp_fold, not 
the other way around.

> > My point is that cp_fold should be a superset of maybe_constant_value, to
> > fix bugs like 53792.  And the easiest way to get that would seem to be by
> > calling maybe_constant_value from cp_fold.
>
> Agreed, that bugs like PR 53792 could be solved that way.
> Nevertheless they aren't directly linked to the delayed-folding
> problem, are they?  We could deal with those cases also in
> genericize-pass lately, as we want to catch here just a missed
> optimization, aren't we?

They aren't due to premature folding, but they are due to the lack of 
delayed folding.  If we're going to fully fold expressions at genericize 
time (and at other times for warnings), that needs to include folding 
calls to constexpr functions, or it isn't "full" folding.  We want to 
handle them together rather than separately because they can interact: 
simplifying one expression with fold may turn it into a C++ 
constant-expression (which is why we have premature-folding bugs), and 
simplifying a constant-expression may expose more opportunities for 
fold.  And we only want a single hash table.

> > OK, that makes sense.  Say, a function called like "fold" that only folds
> > conversions (and NEGATE_EXPR) of constants.  It might make sense to do that
> > and otherwise continue to delay folding of conversions.  In that context I
> > guess this change makes sense.
>
> Fine, any preferences about place for this function?  I suggest the
> name 'fold_cst' for it.

Sounds good.

> > Yes, but we already called maybe_constant_value [in compute_array_index_type].  Calling it again shouldn't
> > make any difference.
>
> We just call it in non-dependent tree,

If the expression is dependent, maybe_constant_value does nothing.

> and even here just for special cases.

Special cases?  It looks like it's called if the expression is not a 
magic C++98 NOP_EXPR with TREE_SIDE_EFFECTS and it is convertible to 
integral or enumeration type.  That seems to cover all cases of constant 
array bounds.

Do you have a testcase that motivates this?

> > > > > -      itype = fold (itype);
> > > > > +      itype = maybe_constant_value (itype);
> > > > > -               itype = variable_size (fold (newitype));
> > > > > +               itype = variable_size (maybe_constant_value (newitype));
> > > >
> > > > Maybe these should use cp_fully_fold?
> > >
> > > We could use fully_fold, but we would also modify none-constant
> > > expressions by this.  Do we actually want that here?
> >
> > For the first one, I actually think a plain fold is enough, or perhaps
> > change the cp_build_binary_op to size_binop.
>
> Hmm, I doubt that a simple fold would be enough here.  As itype is
> calculated as minus of two converts (and cp_converts aren't
> automatically folded).

Sure, the converts need to be folded as well, but we were just saying 
that we are going to automatically fold conversion of a constant, right?

> > > > > @@ -13090,6 +13092,8 @@ build_enumerator (tree name, tree value, tree
> > > > > enumtype, location_t loc)
> > > > > +  if (value)
> > > > > +    value = maybe_constant_value (value);

> > As above, calling the constexpr code twice shouldn't make a difference.
>
> Well, issue is that cxx_constant_value is just called for
> !processing_template_decl case.

Is that important?  Is there a testcase?  Perhaps some change is called 
for, but adding a redundant call to the constexpr code is not that change.

> Additionally we try to
> perform_implicit_conversion_flags on it, which can add new
> cast-operation, which gets resolved by call for cxx_constant_value.

That's after your call to maybe_constant_value, so I don't see how it 
makes any difference.

> > How does delayed folding result in a different OMP_FOR_KIND?
>
> Well,  I guess it is related to optimizations/normalization done early
> on parsing-state, which isn't triggered anymore on later folds.  As
> OMP-stuff isn't that good reading, it is hard to tell why
> normalization doesn't happen here anymore.  I will try to look into
> that, but not sure if it is worth the effort at the very end ...

Hmm, sounds like it could be a significant missed optimization.

Jason