Re: [0/7] Type promotion pass and elimination of zext/sext

[Richard Biener <richard dot guenther at gmail dot com>]

On Sun, Nov 8, 2015 at 10:43 AM, Kugan
<kugan.vivekanandarajah@linaro.org> wrote:
>
> Thanks Richard for the comments.  Please find the attached patches which
> now passes bootstrap with x86_64-none-linux-gnu, aarch64-linux-gnu  and
> ppc64-linux-gnu. Regression testing is ongoing. Please find the comments
> for your questions/suggestions below.
>
>>
>> I notice
>>
>> diff --git a/gcc/tree-ssanames.c b/gcc/tree-ssanames.c
>> index 82fd4a1..80fcf70 100644
>> --- a/gcc/tree-ssanames.c
>> +++ b/gcc/tree-ssanames.c
>> @@ -207,7 +207,8 @@ set_range_info (tree name, enum value_range_type range_type,
>>    unsigned int precision = TYPE_PRECISION (TREE_TYPE (name));
>>
>>    /* Allocate if not available.  */
>> -  if (ri == NULL)
>> +  if (ri == NULL
>> +      || (precision != ri->get_min ().get_precision ()))
>>
>> and I think you need to clear range info on promoted SSA vars in the
>> promotion pass.
>
> Done.
>
>>
>> The basic "structure" thing still remains.  You walk over all uses and
>> defs in all stmts
>> in promote_all_stmts which ends up calling promote_ssa_if_not_promoted on all
>> uses and defs which in turn promotes (the "def") and then fixes up all
>> uses in all stmts.
>
> Done.

Not exactly.  I still see

/* Promote all the stmts in the basic block.  */
static void
promote_all_stmts (basic_block bb)
{
  gimple_stmt_iterator gsi;
  ssa_op_iter iter;
  tree def, use;
  use_operand_p op;

  for (gphi_iterator gpi = gsi_start_phis (bb);
       !gsi_end_p (gpi); gsi_next (&gpi))
    {
      gphi *phi = gpi.phi ();
      def = PHI_RESULT (phi);
      promote_ssa (def, &gsi);

      FOR_EACH_PHI_ARG (op, phi, iter, SSA_OP_USE)
        {
          use = USE_FROM_PTR (op);
          if (TREE_CODE (use) == SSA_NAME
              && gimple_code (SSA_NAME_DEF_STMT (use)) == GIMPLE_NOP)
            promote_ssa (use, &gsi);
          fixup_uses (phi, &gsi, op, use);
        }

you still call promote_ssa on both DEFs and USEs and promote_ssa looks
at SSA_NAME_DEF_STMT of the passed arg.  Please call promote_ssa just
on DEFs and fixup_uses on USEs.

Any reason you do not promote debug stmts during the DOM walk?

So for each DEF you record in ssa_name_info

struct ssa_name_info
{
  tree ssa;
  tree type;
  tree promoted_type;
};

(the fields need documenting).  Add a tree promoted_def to it which you
can replace any use of the DEF with.

Currently as you call promote_ssa for DEFs and USEs you repeatedly
overwrite the entry in ssa_name_info_map with a new copy.  So you
should assert it wasn't already there.

  switch (gimple_code (def_stmt))
    {
    case GIMPLE_PHI:
        {

the last { is indented too much it should be indented 2 spaces
relative to the 'case'


  SSA_NAME_RANGE_INFO (def) = NULL;

only needed in the case 'def' was promoted itself.  Please use
reset_flow_sensitive_info (def).

>>
>> Instead of this you should, in promote_all_stmts, walk over all uses doing what
>> fixup_uses does and then walk over all defs, doing what promote_ssa does.
>>
>> +    case GIMPLE_NOP:
>> +       {
>> +         if (SSA_NAME_VAR (def) == NULL)
>> +           {
>> +             /* Promote def by fixing its type for anonymous def.  */
>> +             TREE_TYPE (def) = promoted_type;
>> +           }
>> +         else
>> +           {
>> +             /* Create a promoted copy of parameters.  */
>> +             bb = single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun));
>>
>> I think the uninitialized vars are somewhat tricky and it would be best
>> to create a new uninit anonymous SSA name for them.  You can
>> have SSA_NAME_VAR != NULL and def _not_ being a parameter
>> btw.
>
> Done. I also had to do some changes to in couple of other places to
> reflect this.
> They are:
> --- a/gcc/tree-ssa-reassoc.c
> +++ b/gcc/tree-ssa-reassoc.c
> @@ -302,6 +302,7 @@ phi_rank (gimple *stmt)
>      {
>        tree arg = gimple_phi_arg_def (stmt, i);
>        if (TREE_CODE (arg) == SSA_NAME
> +         && SSA_NAME_VAR (arg)
>           && !SSA_NAME_IS_DEFAULT_DEF (arg))
>         {
>           gimple *def_stmt = SSA_NAME_DEF_STMT (arg);
> @@ -434,7 +435,8 @@ get_rank (tree e)
>        if (gimple_code (stmt) == GIMPLE_PHI)
>         return phi_rank (stmt);
>
> -      if (!is_gimple_assign (stmt))
> +      if (!is_gimple_assign (stmt)
> +         && !gimple_nop_p (stmt))
>         return bb_rank[gimple_bb (stmt)->index];
>
> and
>
> --- a/gcc/tree-ssa.c
> +++ b/gcc/tree-ssa.c
> @@ -752,7 +752,8 @@ verify_use (basic_block bb, basic_block def_bb,
> use_operand_p use_p,
>    TREE_VISITED (ssa_name) = 1;
>
>    if (gimple_nop_p (SSA_NAME_DEF_STMT (ssa_name))
> -      && SSA_NAME_IS_DEFAULT_DEF (ssa_name))
> +      && (SSA_NAME_IS_DEFAULT_DEF (ssa_name)
> +         || SSA_NAME_VAR (ssa_name) == NULL))
>      ; /* Default definitions have empty statements.  Nothing to do.  */
>    else if (!def_bb)
>      {
>
> Does this look OK?

Hmm, no, this looks bogus.

I think the best thing to do is not promoting default defs at all and instead
promote at the uses.

              /* Create a promoted copy of parameters.  */
              bb = single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun));
              gcc_assert (bb);
              gsi2 = gsi_after_labels (bb);
              new_def = copy_ssa_name (def);
              set_ssa_promoted (new_def);
              set_ssa_default_def (cfun, SSA_NAME_VAR (def), new_def);
              duplicate_default_ssa (new_def, def);
              TREE_TYPE (def) = promoted_type;

AFAIK this is just an awkward way of replacing all uses by a new DEF, sth
that should be supported by the machinery so that other default defs can just
do

             new_def = get_or_create_default_def (create_tmp_reg
(promoted_type));

and have all uses ('def') replaced by new_def.

>>
>> +/* Return true if it is safe to promote the defined SSA_NAME in the STMT
>> +   itself.  */
>> +static bool
>> +safe_to_promote_def_p (gimple *stmt)
>> +{
>> +  enum tree_code code = gimple_assign_rhs_code (stmt);
>> +  if (gimple_vuse (stmt) != NULL_TREE
>> +      || gimple_vdef (stmt) != NULL_TREE
>> +      || code == ARRAY_REF
>> +      || code == LROTATE_EXPR
>> +      || code == RROTATE_EXPR
>> +      || code == VIEW_CONVERT_EXPR
>> +      || code == BIT_FIELD_REF
>> +      || code == REALPART_EXPR
>> +      || code == IMAGPART_EXPR
>> +      || code == REDUC_MAX_EXPR
>> +      || code == REDUC_PLUS_EXPR
>> +      || code == REDUC_MIN_EXPR)
>> +    return false;
>> +  return true;
>>
>> huh, I think this function has an odd name, maybe
>> can_promote_operation ()?  Please
>> use TREE_CODE_CLASS (code) == tcc_reference for all _REF trees.
>
> Done.
>
>>
>> Note that as followup things like the rotates should be "expanded" like
>> we'd do on RTL (open-coding the thing).  And we'd need a way to
>> specify zero-/sign-extended loads.
>>
>> +/* Return true if it is safe to promote the use in the STMT.  */
>> +static bool
>> +safe_to_promote_use_p (gimple *stmt)
>> +{
>> +  enum tree_code code = gimple_assign_rhs_code (stmt);
>> +  tree lhs = gimple_assign_lhs (stmt);
>> +
>> +  if (gimple_vuse (stmt) != NULL_TREE
>> +      || gimple_vdef (stmt) != NULL_TREE
>>
>> I think the vuse/vdef check is bogus, you can have a use of 'i_3' in say
>> _2 = a[i_3];
>>
> When I remove this, I see errors in stmts like:
>
> unsigned char
> unsigned int
> # .MEM_197 = VDEF <.MEM_187>
> fs_9(D)->fde_encoding = _154;

Yeah, as said a stmt based check is really bogus without context.  As the
predicate is only used in a single place it's better to inline it
there.  In this
case you want to handle loads/stores differently.  From this context it
looks like not iterating over uses in the caller but rather iterating over
uses here makes most sense as you then can do

   if (gimple_store_p (stmt))
     {
        promote all uses that are not gimple_assign_rhs1 ()
     }

you can also transparently handle constants for the cases where promoting
is required.  At the moment their handling is interwinded with the def promotion
code.  That makes the whole thing hard to follow.

Thanks,
Richard.

>
>> +      || code == VIEW_CONVERT_EXPR
>> +      || code == LROTATE_EXPR
>> +      || code == RROTATE_EXPR
>> +      || code == CONSTRUCTOR
>> +      || code == BIT_FIELD_REF
>> +      || code == COMPLEX_EXPR
>> +      || code == ASM_EXPR
>> +      || VECTOR_TYPE_P (TREE_TYPE (lhs)))
>> +    return false;
>> +  return true;
>>
>> ASM_EXPR can never appear here.  I think PROMOTE_MODE never
>> promotes vector types - what cases did you need to add VECTOR_TYPE_P for?
>
> Done
>>
>> +/* Return true if the SSA_NAME has to be truncated to preserve the
>> +   semantics.  */
>> +static bool
>> +truncate_use_p (gimple *stmt)
>> +{
>> +  enum tree_code code = gimple_assign_rhs_code (stmt);
>>
>> I think the description can be improved.  This is about stray bits set
>> beyond the original type, correct?
>>
>> Please use NOP_EXPR wherever you use CONVERT_EXPR right how.
>>
>> +                 if (TREE_CODE_CLASS (code)
>> +                     == tcc_comparison)
>> +                   promote_cst_in_stmt (stmt, promoted_type, true);
>>
>> don't you always need to promote constant operands?
>
> I am promoting all the constants. Here, I am promoting the the constants
> that are part of the conditions.
>
>
> Thanks,
> Kugan