This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: C++ delayed folding branch review

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);



Within the constexpr code we should be folding away NOPs as they are
generated, they shouldn't live this long.



Well, we might see them on overflows ...



We shouldn't within the constexpr code.  NOPs for expressions that are
non-constant due to overflow are added in
cxx_eval_outermost_constant_expr, so we shouldn't see them in the middle
of constexpr evaluation.


^


@@ -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.



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?



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.


^


@@ -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.


^


@@ -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.


^


@@ -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.


^


@@ -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?


^


@@ -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.


^


@@ -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.


@@ -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.


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. 


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?


@@ -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? 


@@ -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.


?


@@ -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.


Yes, they are.  We handle now the avoiding of dead-code for constants
(cond-expression, truthif* expressions, and useless convert-warnings).
So this change is something only interesting for different compiler,
but not related to delayed-folding anymore.
I will check, and remove it tomorrow.


Looks like this is still there.


@@ -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.


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. 


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.


^


@@ -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.


^


@@ -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.


^

Jason
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]