> > Patches attached. They already incorporated the .cc rename, now
> > rebased to be after the change to tree.h
> 
> @@ -1412,8 +1412,7 @@ vect_recog_widen_op_pattern (vec_info *vinfo,
>                        2, oprnd, half_type, unprom, vectype);
> 
>    tree var = vect_recog_temp_ssa_var (itype, NULL);
> -  gimple *pattern_stmt = gimple_build_assign (var, wide_code,
> -                                             oprnd[0], oprnd[1]);
> +  gimple *pattern_stmt = gimple_build (var, wide_code, oprnd[0],
> oprnd[1]);
> 
> 
> you should be able to do without the new gimple_build overload
> by using
> 
>    gimple_seq stmts = NULL;
>    gimple_build (&stmts, wide_code, itype, oprnd[0], oprnd[1]);
>    gimple *pattern_stmt = gimple_seq_last_stmt (stmts);
> 
> because 'gimple_build' is an existing API.

Done.

The gimple_build overload was at the request of Richard Sandiford, I assume 
removing it is ok with you Richard S?
>From Richard Sandiford:
> For example, I think we should hide this inside a new:
> 
>   gimple_build (var, wide_code, oprnd[0], oprnd[1]);
> 
> that works directly on code_helper, similarly to the new code_helper 
> gimple_build interfaces.




> 
> 
> -  if (TREE_CODE (gimple_assign_lhs (stmt)) != SSA_NAME)
> +  if (gimple_get_lhs (stmt) == NULL_TREE ||
> +      TREE_CODE(gimple_get_lhs (stmt)) != SSA_NAME)
>      return false;
> 
> || go to the next line, space after TREE_CODE
> 

Done.

> +  bool widen_arith = false;
> +  gimple_match_op res_op;
> +  if (!gimple_extract_op (stmt, &res_op))
> +    return false;
> +  code = res_op.code;
> +  op_type = res_op.num_ops;
> +
> +  if (is_gimple_assign (stmt))
> +  {
> +      widen_arith = (code == WIDEN_PLUS_EXPR
> +                    || code == WIDEN_MINUS_EXPR
> +                    || code == WIDEN_MULT_EXPR
> +                    || code == WIDEN_LSHIFT_EXPR);
> + }
> +  else
> +      widen_arith = gimple_call_flags (stmt) & ECF_WIDEN;
> 
> there seem to be formatting issues.  Also shouldn't you check
> if (res_op.code.is_tree_code ()) instead if is_gimple_assign?
> I also don't like the ECF_WIDEN "trick", just do as with the
> tree codes and explicitely enumerate widening ifns here.
> 

Done. I've set widen_arith to False for the first patch as the second patch 
introduces the widening ifns.

> gimple_extract_op is a bit heavy-weight as well, so maybe
> instead simply do
> 
>   if (is_gimple_assign (stmt))
>     {
>       code = gimple_assign_rhs_code (stmt);
> ...
>     }
>   else if (gimple_call_internal_p (stmt))
>     {
>       code = gimple_call_internal_fn (stmt);
> ...
>     }
>   else
>     return false;

The patch was originally written as above, it was changed to use 
gimple_extract_op at the request of Richard Sandiford. I prefer 
gimple_extract_op as it's more compact, but I don't have strong feelings. If 
the Richards can agree on either version I'm happy.


>From Richard Sandiford:
> > +  if (is_gimple_assign (stmt))
> > +  {
> > +    code_or_ifn = gimple_assign_rhs_code (stmt);  }  else
> > +    code_or_ifn = gimple_call_combined_fn (stmt);
> 
> It might be possible to use gimple_extract_op here (only recently added).
> This would also provide the number of operands directly, instead of 
> needing "op_type".  It would also provide an array of operands.


> 
> +  code_helper c1=MAX_TREE_CODES, c2=MAX_TREE_CODES;
> 
> spaces before/after '='
> 

Done.

> @@ -12061,13 +12105,16 @@ supportable_widening_operation (vec_info
> *vinfo,
>    if (BYTES_BIG_ENDIAN && c1 != VEC_WIDEN_MULT_EVEN_EXPR)
>      std::swap (c1, c2);
> 
> +
>    if (code == FIX_TRUNC_EXPR)
>      {
> 
> unnecessary whitespace change.
> 
Fixed.

> diff --git a/gcc/tree.h b/gcc/tree.h
> index
> f84958933d51144bb6ce7cc41eca5f7f06814550..e51e34c051d9b91d1c02a4b2
> fefdb2b15606a36f
> 100644
> --- a/gcc/tree.h
> +++ b/gcc/tree.h
> @@ -92,6 +92,10 @@ public:
>    bool is_fn_code () const { return rep < 0; }
>    bool is_internal_fn () const;
>    bool is_builtin_fn () const;
> +  enum tree_code as_tree_code () const { return is_tree_code () ?
> +    (tree_code)* this : MAX_TREE_CODES; }
> +  combined_fn as_fn_code () const { return is_fn_code () ? (combined_fn)
> *this
> +    : CFN_LAST;}
> 
> hmm, the other as_* functions we have are not member functions.
> Also this semantically differs from the tree_code () conversion
> operator (that one was supposed to be "cheap").  The existing
> as_internal_fn for example is documented as being valid only if
> the code is actually an internal fn.  I see you are introducing
> the new function as convenience to get a "safe" not-a-X value,
> so maybe they should be called safe_as_tree_code () instead?
> 
SGTM. Done

> 
>    int get_rep () const { return rep; }
>    bool operator== (const code_helper &other) { return rep == other.rep; }
>    bool operator!= (const code_helper &other) { return rep != other.rep; }
> @@ -6657,6 +6661,54 @@ extern unsigned fndecl_dealloc_argno (tree);
>     if nonnull, set the second argument to the referenced enclosing
>     object or pointer.  Otherwise return null.  */
>  extern tree get_attr_nonstring_decl (tree, tree * = NULL);
> +/* Helper to transparently allow tree codes and builtin function codes
> +   exist in one storage entity.  */
> +class code_helper
> +{
> 
> duplicate add of code_helper.
Fixed.


Tests are being re-run.

Ok, with changes?

Attachment: 0001-Refactor-to-allow-internal_fn-s.patch
Description: 0001-Refactor-to-allow-internal_fn-s.patch

Attachment: 0002-Refactor-widen_plus-as-internal_fn.patch
Description: 0002-Refactor-widen_plus-as-internal_fn.patch

Attachment: 0003-Remove-widen_plus-minus_expr-tree-codes.patch
Description: 0003-Remove-widen_plus-minus_expr-tree-codes.patch

Reply via email to