Re: [PATCH 1/2] Mid engine setup [SU]ABDL

[Oluwatamilore Adebayo <oluwatamilore.adebayo@arm.com>]

> Sorry, my fault.  I was using the original type names in this
> suggestion, rather than the TYPE1…TYPE5 ones.  Should be:
> 
>    WIDEN_ABD exists to optimize the case where TYPE4 is at least
>    twice as wide as TYPE3.

Change made.

> Lingering use of “L” suffixes here.  Maybe:
> 
>      stmts that constitute the pattern, principally:
> 	out = IFN_ABD (x, y)
> 	out = IFN_WIDEN_ABD (x, y)

Change made.

> > +  if (TYPE_PRECISION (out_type) >= TYPE_PRECISION (abd_in_type) * 2
> > +      && TYPE_PRECISION (abd_out_type) != stmt_vinfo->min_output_precision)
> 
> Sorry for not noticing last time, but I think the second condition
> would be more natural as:
> 
>       && stmt_vinfo->min_output_precision >= TYPE_PRECISION (abd_in_type) * 2)
> 
> (There's no distinction between abs_in_type and abs_out_type at this point,
> so it seems clearer to use the same value in both conditions.)

Change made.

> > +  gassign *last_stmt = dyn_cast <gassign *> (STMT_VINFO_STMT (stmt_vinfo));
> > +  if (!last_stmt || !gimple_assign_cast_p (last_stmt))
> 
> I think this should be:
> 
>   if (!last_stmt || !CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (last_stmt)))
> 
> gimple_assign_cast_p is more general, and allows conversions
> between integral and non-integral types.

Change made.

> > +  tree in_type = TREE_TYPE (last_rhs);
> > +  tree out_type = TREE_TYPE (gimple_assign_lhs (last_stmt));
> > +  if (TYPE_PRECISION (in_type) * 2 != TYPE_PRECISION (out_type))
> > +    return NULL;
> 
> I think this also needs to require TYPE_UNSIGNED (in_type):
> 
>   if (TYPE_PRECISION (in_type) * 2 != TYPE_PRECISION (out_type)
>       || !TYPE_UNSIGNED (in_type))
>     return NULL;
> 
> That is, the extension has to be a zero extension rather than
> a sign extension.
> 
> For example:
> 
>   int32_t a, b, c;
>   int64_t d;
> 
>   c = IFN_ABD (a, b);
>   d = (int64_t) c;
> 
> sign-extends the ABD result to 64 bits, and so a == INT_MAX
> && b == INT_MIN gives:
> 
>   c = -1    (UINT_MAX converted to signed)
>   d = -1
> 
> But IFN_WIDEN_ABD would give d == UINT_MAX instead.

Change made.

> > +  gimple *pattern_stmt = STMT_VINFO_STMT (abd_pattern_vinfo);
> > +  if (gimple_assign_cast_p (pattern_stmt))
> > +    {
> > +       tree op = gimple_assign_rhs1 (pattern_stmt);
> > +       vect_unpromoted_value unprom;
> > +       op = vect_look_through_possible_promotion (vinfo, op, &unprom);
> > +
> > +      if (!op)
> > +	return NULL;
> > +
> > +      abd_pattern_vinfo = vect_get_internal_def (vinfo, op);
> > +      if (!abd_pattern_vinfo)
> > +	return NULL;
> > +
> > +      pattern_stmt = STMT_VINFO_STMT (abd_pattern_vinfo);
> > +    }
> 
> I think the code quoted above reduces to:
> 
>   vect_unpromoted_value unprom;
>   tree op = vect_look_through_possible_promotion (vinfo, last_rhs, &unprom);
>   if (!op || TYPE_PRECISION (TREE_TYPE (op)) != TYPE_PRECISION (in_type))
>     return NULL;
> 
>   stmt_vec_info abd_pattern_vinfo = vect_get_internal_def (vinfo, op);
>   if (!abd_pattern_vinfo)
>     return NULL;
>   abd_pattern_vinfo = vect_stmt_to_vectorize (abd_pattern_vinfo);
> 
> ...
> 
> > +  tree abd_oprnd0 = gimple_call_arg (abd_stmt, 0);
> > +  tree abd_oprnd1 = gimple_call_arg (abd_stmt, 1);
> > +  if (TYPE_PRECISION (TREE_TYPE (abd_oprnd0)) != TYPE_PRECISION (in_type))
> > +    return NULL;
> 
> With the changes above, this check would not be necessary.

Both changes made.

Updated patch will be in the next email.