Re: condition DCE of partially dead calls

[Xinliang David Li <davidxl at google dot com>]

Diego Novillo wrote:
> On Fri, May 30, 2008 at 18:39, Xinliang David Li <davidxl@google.com> wrote:
>
> Arguments need documenting.

Done.

> s/method/function/  This is also in other places.  In GCC, we use
> 'function' instead of 'method'.

Changed.

> > +static bool
> > +check_target_format (tree arg)
> > +{
> > +  tree typ;
>
> Any particular reason why you spelled it 'typ'?

Just an old habit -- changed to type.


> Wouldn't it be cheaper to do this check once on startup?  Traverse all
> the modes and make sure that they are the ones we want to handle here.
> You mention that this check is very cheap, so perhaps this is not
> necessary.  However, all these tests are not so much dependent on
> ARG as they are on the target modes.

Yes, a little. It will need to remember result per type though.



> > +/* A helper method to help select calls to pow that are suitable for
> > +   conditional DCE transformation.  It looks for pow calls that can be
> > +   guided with simple conditions.  Such calls either have constant base
> > +   values or base values converted from integers.  Returns true if
> > +   the pow call is a candidate.  */
>
> s/the pow call/the pow call POW_CALL/

Changed.

> > +      /* Only handles cases where base value is converted
> > +         from integer values.  */
> > +      base_def = SSA_NAME_DEF_STMT (base);
> > +      if (TREE_CODE (base_def) != GIMPLE_MODIFY_STMT)
> > +        return false;
> > +
> > +      base_val = GIMPLE_STMT_OPERAND (base_def, 1);
> > +
> > +      if (TREE_CODE (base_val) != FLOAT_EXPR)
> > +        return false;
> > +      base_val0 = TREE_OPERAND (base_val, 0);
> > +
> > +      if (TREE_CODE (base_val0) != SSA_NAME)
> > +        return false;
>
> This can never happen.  base_val0 == base, always.

Removed.

> > +      /* If the type of the base is too wide,
> > +         the resulting shrink wrapping condition
> > +	 will be too conservative.  */
> > +      if (bit_sz > 32)
> > +        return false;
>
> Why 32, though?

Empirical number.

> > +  temp = create_tmp_var (float_typ, temp_name1);
> > +  stmt1 = build_gimple_modify_stmt (temp, arg);
> > +  tempn = make_ssa_name (temp, stmt1);
> > +  GIMPLE_STMT_OPERAND (stmt1, 0) = tempn;
> > +
> > +  tempc = create_tmp_var (boolean_type_node, temp_name2);
> > +  stmt2 = build_gimple_modify_stmt (tempc,
> > +                                    build2 (tcode,
> > +                                            boolean_type_node,
> > +                                            tempn, lbub_real_cst));
> > +  tempcn = make_ssa_name (tempc, stmt2);
> > +  GIMPLE_STMT_OPERAND (stmt2, 0) = tempcn;
> > +
> > +  stmt3 = build3 (COND_EXPR, void_type_node,
> > +                  tempcn, NULL_TREE, NULL_TREE);
>
> Why not generate COND_EXPR <ARG TCODE LBUB_REAL_CST, NULL, NULL>
> directly?  Fewer temporaries.

In the spirit of generating small trees :)

> > +  VEC_safe_push (tree, heap, conds, stmt1);
> > +  VEC_safe_push (tree, heap, conds, stmt2);
> > +  VEC_safe_push (tree, heap, conds, stmt3);
>
> You are using VEC_safe_push here but CONDS is passed by value.
> If VEC_safe_push needs to resize it, this will fail.  Either pass
> CONDS by reference or use VEC_quick_push which will assert that
> the element fits in the array.

Added assert on *CONDS size before but removed.  Changed to use
quick_push.


> > +static void
> > +gen_conditions_for_domain (tree arg, inp_domain domain,
> > +                           VEC (tree, heap) *conds,
> > +                           unsigned * nconds)
>
> s/unsigned * nconds/unsigned *nconds/

Changed.

> > +  if (domain.has_ub)
> > +    {
> > +      /* Now push a separator.  */
> > +      if (domain.has_lb)
> > +        VEC_safe_push (tree, heap, conds, NULL);
> > +
>
> Likewise here.  Use VEC_quick_push or pass CONDS by reference.

Done.

> > +/* A helper function to generate condition
> > +   code for the y argument in call pow (some_const, y).
> > +   See candidate selection in check_pow.  Since the
> > +   candidates' base values have a limited range,
> > +   the guarded code generated for y are simple:
> > +   if (y > max_y)
> > +     pow (const, y);
> > +   Note max_y can be computed separately for each
> > +   const base, but in this implementation, we
> > +   choose to compute it using the max base
> > +   in the allowed range for the purpose of
> > +   simplicity.  */
> > +
> > +static void
> > +gen_conditions_for_pow_cst_base (tree base, tree expn,
> > +                                 VEC (tree, heap) *conds,
> > +                                 unsigned *nconds)
>
> Arguments need documenting.

Done.


> > +{
> > +  inp_domain exp_domain;
> > +  /* Validate the range of the base constant to make
> > +     sure of consistency with check_pow.  */
>
> s/of consistency/it is consistent/

Done.

> > +/* Generate error condition code for pow calls with
> > +   non constant base values.  The candidates selected
> > +   have their base argument value converted from
> > +   integer (see check_pow) value (1, 2, 4 bytes), and
> > +   the max exp value is computed based on the size
> > +   of the integer type (i.e. max possible base value).
> > +   The resulting input domain for exp argument is thus
> > +   conservative (smaller than the max value allowed by
> > +   the runtime value of the base).  */
> > +
> > +static void
> > +gen_conditions_for_pow_int_base (tree base, tree expn,
> > +                                 VEC (tree, heap) *conds,
> > +                                 unsigned *nconds)
>
> Arguments need documenting.

Done.

> > +  base_def = SSA_NAME_DEF_STMT (base);
> > +  base_nm = GIMPLE_STMT_OPERAND (base_def, 0);
> > +  base_val = GIMPLE_STMT_OPERAND (base_def, 1);
> > +  base_val0 = TREE_OPERAND (base_val, 0);
> > +  base_var = SSA_NAME_VAR (base_val0);
> > +  int_typ = TREE_TYPE (base_var);
> > +  bit_sz = TYPE_PRECISION (int_typ);
> > +  gcc_assert (bit_sz > 0 && bit_sz <= 32);
>
> Is 32 a constant that may/will change depending on target or
> other conditions?  Perhaps it should be either a #define constant
> or a compiler parameter or read from target attributes.

Defined a macro for it.


> > +  /* Push a separator.  */
> > +  VEC_safe_push (tree, heap, conds, NULL);
>
> Similar comments as before.  This needs to be VEC_quick_push or
> CONDS passed in by reference.

Done.

> > +/* A helper routine to help computing the valid input domain
> > +   for a built-in function.  See C99 7.12.7 for details.  In this
>
> s/a built-in function/builtin function FNC/
Done.

> > +/* The method to generate shrink wrap conditions for a partially
>
> s/The method to generate/Generate/
>
> > +/* The method to shrink wrap a partially dead builtin call
>
> s/The method to shrink/Shrink wrap/

Changed to 'the Function'.

> > +  /* This can happen if the condition generator decides
> > +     it is not beneficial to do the transformation.  Just
> > +     return false and does not do any transformation for
>
> s/does not/do not/

Done.

> > +          /* Ideally the function call conditional
> > +             dead code elimination phase can be delayed
> > +             till later where potentially more opportunities
> > +             can be found.  Due to lack of good ways to
> > +             update VDEFs associated with the shrink-wrapped
> > +             calls, it is better to do the transformation
> > +             here where memory SSA is not built yet.  */
>
> Well, you could mark all the symbols in the VDEF/VUSE operands
> for renaming.  In fact, that's pretty much all you can do, unless
> you patch the FUD chains manually.

Yes. The additional benefit may be very low though.

> The patch looks OK with these changes.  Unless there are other
> outstanding comments, it's fine to commit.
>
> Thanks for your patience in this review cycle.  The
> transformation looks pretty decent.


Thanks, will commit after final testing.


David


> Diego.