RE: [ping][vect-patterns] Refactor widen_plus/widen_minus as internal_fns

[Richard Biener <rguenther@suse.de>]

On Thu, 30 Jun 2022, Joel Hutton wrote:

> > We can go with a private vect_gimple_build function until we sort out the API
> > issue to unblock Tamar (I'll reply to Richards reply with further thoughts on
> > this)
> > 
> 
> Done.
> 
> > > Similarly are you ok with the use of gimple_extract_op? I would lean
> > towards using it as it is cleaner, but I don't have strong feelings.
> > 
> > I don't like using gimple_extract_op here, I think I outlined a variant that is
> > even shorter.
> > 
> 
> Done.
> 
> Updated patches attached, bootstrapped and regression tested on aarch64.
> 
> Tomorrow is my last working day at Arm, so it will likely be Andre that commits this/addresses any further comments.

First sorry for the (repeated) delays.

In the first patch I still see ECF_WIDEN, I don't like that, we
use things like associative_binary_fn_p so for widening internal
functions similar predicates should be used.

In the second patch you add vec_widen_{add,sub} optabs

+OPTAB_CD(vec_widen_add_optab, "add$a$b3")
+OPTAB_CD(vec_widen_sub_optab, "sub$a$b3")

but a) the names are that of regular adds which is at least confusing
(if not wrong), b) there's no documentation for them in md.texi which,
c) doesn't explain why they are necessary when we have 
vec_widen_[su]{add,sub}l_optab

+      internal_fn ifn = as_internal_fn (code.safe_as_fn_code ());

asks for safe_as_internal_fn () (just complete the API, also with
safe_as_builtin_fn)

+      internal_fn lo, hi;
+      lookup_multi_internal_fn (ifn, &lo, &hi);
+      *code1 = as_combined_fn (lo);
+      *code2 = as_combined_fn (hi);

in fact this probably shows that the guarding condition should
be if (code.is_internal_fn ()) instead of if (code.is_fn_code ()).

+      optab1 = lookup_multi_ifn_optab (lo, !TYPE_UNSIGNED (vectype));
+      optab2 = lookup_multi_ifn_optab (hi, !TYPE_UNSIGNED (vectype));

this shows the two lookup_ APIs are inconsistent in having two vs. one
output, please make them consistent.  I'd say give
lookup_multi_internal_fn a enum { LO, HI } argument, returning the
result.  Given VEC_WIDEN_MULT has HI, LO, EVEN and ODD variants
that sounds more future proof.

The internal_fn stuff could probably get a 2nd eye from Richard.

In the third patch I see unrelated and wrong changes like

          /* Check that the DR_INITs are compile-time constants.  */
-         if (!tree_fits_shwi_p (DR_INIT (dra))
-             || !tree_fits_shwi_p (DR_INIT (drb)))
+         if (TREE_CODE (DR_INIT (dra)) != INTEGER_CST
+             || TREE_CODE (DR_INIT (drb)) != INTEGER_CST)
            break;

please strip the patch down to relevant changes.

-      tree op = gimple_op (assign, i + 1);
+      tree op;
+      if (is_gimple_assign (stmt))
+       op = gimple_op (stmt, i + 1);
+      else
+       op = gimple_call_arg (stmt, i);

somebody added gimple_arg which can be used here doing

       op = gimple_arg (stmt, i);

+  tree lhs = is_gimple_assign (stmt) ? gimple_assign_lhs (stmt):
+                                     gimple_call_lhs (stmt);

  tree lhs = gimple_get_lhs (stmt);

   /* Check for an integer operation with the right code.  */
-  gassign *assign = dyn_cast <gassign *> (stmt_info->stmt);
-  if (!assign)
+  gimple* stmt = stmt_info->stmt;
+  if (!(is_gimple_assign (stmt) || is_gimple_call (stmt)))
     return 0;

-  tree_code rhs_code = gimple_assign_rhs_code (assign);
-  if (rhs_code != code && rhs_code != widened_code)
+  code_helper rhs_code;
+  if (is_gimple_assign (stmt))
+    rhs_code = gimple_assign_rhs_code (stmt);
+  else
+    rhs_code = gimple_call_combined_fn (stmt);
+
+  if (rhs_code.safe_as_tree_code () != code
+      && rhs_code.get_rep () != widened_code.get_rep ())
     return 0;

that's probably better refactored as

 if (is_gimple_assign (stmt))
   {
     if (code check)
       return 0;
   }
 else if (is_gimple_call (..))
  {
  ..
  }
 else
   return 0;

otherwise the last patch looks reasonable.

Richard.