This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH, rs6000] (v2) GIMPLE folding for vector compares
- From: Segher Boessenkool <segher at kernel dot crashing dot org>
- To: Will Schmidt <will_schmidt at vnet dot ibm dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>, Richard Biener <richard dot guenther at gmail dot com>, Bill Schmidt <wschmidt at linux dot vnet dot ibm dot com>, David Edelsohn <dje dot gcc at gmail dot com>
- Date: Wed, 15 Nov 2017 07:33:02 -0600
- Subject: Re: [PATCH, rs6000] (v2) GIMPLE folding for vector compares
- Authentication-results: sourceware.org; auth=none
- References: <1510697494.26707.252.camel@brimstone.rchland.ibm.com>
Hi Will,
On Tue, Nov 14, 2017 at 04:11:34PM -0600, Will Schmidt wrote:
> Add support for gimple folding of vec_cmp_{eq,ge,gt,le,ne}
> for the integer data types.
The code looks fine, just some typographical stuff:
> * config/rs6000/vsx.md (vcmpneb, vcmpneh, vcmpnew): Update to specify
> the not+eq combination.
Trailing space.
> +/* Helper function to handle the gimple folding of a vector compare
> + operation. This sets up true/false vectors, and uses the
> + VEC_COND_EXPR operation.
> + 'code' indicates which comparison is to be made. (EQ, GT, ...).
> + 'type' indicates the type of the result. */
One space less in the comment indent. Names of parameters are written in
CAPS, no quotes.
> +static void
> +fold_compare_helper (gimple_stmt_iterator *gsi, tree_code code, gimple *stmt)
> +{
> + tree arg0 = gimple_call_arg (stmt, 0);
> + tree arg1 = gimple_call_arg (stmt, 1);
> + tree lhs = gimple_call_lhs (stmt);
> + gimple *g = gimple_build_assign (lhs,
> + fold_build_vec_cmp (code, TREE_TYPE (lhs), arg0, arg1));
That's not the standard indenting. Maybe break the statement to make
it easier? I.e.
tree cmp = fold_build_vec_cmp (code, TREE_TYPE (lhs), arg0, arg1);
gimple *g = gimple_build_assign (lhs, cmp);
> @@ -16701,10 +16731,67 @@ rs6000_gimple_fold_builtin (gimple_stmt_iterator *gsi)
> gimple_set_location (g, gimple_location (stmt));
> gsi_replace (gsi, g, true);
> return true;
> }
>
> + /* Vector compares; EQ, NE, GE, GT, LE. */
> + case ALTIVEC_BUILTIN_VCMPEQUB:
> + case ALTIVEC_BUILTIN_VCMPEQUH:
> + case ALTIVEC_BUILTIN_VCMPEQUW:
> + case P8V_BUILTIN_VCMPEQUD:
> + {
> + fold_compare_helper (gsi, EQ_EXPR, stmt);
> + return true;
> + }
There's no need to make a block here (a bunch more of this later).
> @@ -18260,10 +18347,27 @@ builtin_function_type (machine_mode mode_ret, machine_mode mode_arg0,
> case MISC_BUILTIN_UNPACK_TD:
> case MISC_BUILTIN_UNPACK_V1TI:
> h.uns_p[0] = 1;
> break;
>
> + /* unsigned arguments, bool return (compares). */
> + case ALTIVEC_BUILTIN_VCMPEQUB:
The comment indent is wrong.
> /* unsigned arguments for 128-bit pack instructions. */
> case MISC_BUILTIN_PACK_TD:
Here too, but that is existing code :-)
Okay for trunk with those trivialities cleaned up. Thanks!
Segher