* [PATCH] Don't ICE on long long shifts in vectorizable_shift
@ 2011-10-27 20:23 Jakub Jelinek
2011-10-28 8:58 ` Richard Guenther
0 siblings, 1 reply; 4+ messages in thread
From: Jakub Jelinek @ 2011-10-27 20:23 UTC (permalink / raw)
To: Ira Rosen, Richard Guenther; +Cc: gcc-patches
Hi!
With the patch I'm going to post momentarily which adds vlshrv{4,2}di and
vashlv{4,2}di patterns for -mavx2 vectorizable_shift ICEs, because the
frontends for long_long_var1 << long_long_var2 emit long_long_var1 << (int) long_long_var2
and vectorizable_shift isn't prepared to handle type promotion (or
demotion). IMHO it would complicate it too much, so this patch just
gives up on vectorizing in that case.
I can work on Monday on pattern recognizer that will change
shifts/rotates where the rhs1 has different size from rhs2 into
a pattern with def_stmt casting the rhs2 to the same type as rhs1
and pattern_stmt that uses the temporary for rhs2, perhaps with extra
optimization if it sees the type being promoted/demoted again to just look
through those.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2011-10-27 Jakub Jelinek <jakub@redhat.com>
* tree-vect-stmts.c (vectorizable_shift): Give up if op1 has different
vector mode from vectype's mode.
--- gcc/tree-vect-stmts.c.jj 2011-10-27 08:42:51.000000000 +0200
+++ gcc/tree-vect-stmts.c 2011-10-27 17:24:15.000000000 +0200
@@ -2318,6 +2318,7 @@ vectorizable_shift (gimple stmt, gimple_
int nunits_in;
int nunits_out;
tree vectype_out;
+ tree op1_vectype;
int ncopies;
int j, i;
VEC (tree, heap) *vec_oprnds0 = NULL, *vec_oprnds1 = NULL;
@@ -2387,7 +2388,8 @@ vectorizable_shift (gimple stmt, gimple_
return false;
op1 = gimple_assign_rhs2 (stmt);
- if (!vect_is_simple_use (op1, loop_vinfo, bb_vinfo, &def_stmt, &def, &dt[1]))
+ if (!vect_is_simple_use_1 (op1, loop_vinfo, bb_vinfo, &def_stmt, &def,
+ &dt[1], &op1_vectype))
{
if (vect_print_dump_info (REPORT_DETAILS))
fprintf (vect_dump, "use not simple.");
@@ -2444,6 +2446,13 @@ vectorizable_shift (gimple stmt, gimple_
optab = optab_for_tree_code (code, vectype, optab_vector);
if (vect_print_dump_info (REPORT_DETAILS))
fprintf (vect_dump, "vector/vector shift/rotate found.");
+ if (TYPE_MODE (op1_vectype) != TYPE_MODE (vectype))
+ {
+ if (vect_print_dump_info (REPORT_DETAILS))
+ fprintf (vect_dump, "unusable type for last operand in"
+ " vector/vector shift/rotate.");
+ return false;
+ }
}
/* See if the machine has a vector shifted by scalar insn and if not
then see if it has a vector shifted by vector insn. */
Jakub
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Don't ICE on long long shifts in vectorizable_shift
2011-10-27 20:23 [PATCH] Don't ICE on long long shifts in vectorizable_shift Jakub Jelinek
@ 2011-10-28 8:58 ` Richard Guenther
2011-10-28 9:21 ` Jakub Jelinek
0 siblings, 1 reply; 4+ messages in thread
From: Richard Guenther @ 2011-10-28 8:58 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Ira Rosen, gcc-patches
[-- Attachment #1: Type: TEXT/PLAIN, Size: 3497 bytes --]
On Thu, 27 Oct 2011, Jakub Jelinek wrote:
> Hi!
>
> With the patch I'm going to post momentarily which adds vlshrv{4,2}di and
> vashlv{4,2}di patterns for -mavx2 vectorizable_shift ICEs, because the
> frontends for long_long_var1 << long_long_var2 emit long_long_var1 << (int) long_long_var2
> and vectorizable_shift isn't prepared to handle type promotion (or
> demotion). IMHO it would complicate it too much, so this patch just
> gives up on vectorizing in that case.
>
> I can work on Monday on pattern recognizer that will change
> shifts/rotates where the rhs1 has different size from rhs2 into
> a pattern with def_stmt casting the rhs2 to the same type as rhs1
> and pattern_stmt that uses the temporary for rhs2, perhaps with extra
> optimization if it sees the type being promoted/demoted again to just look
> through those.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Hm, but you are testing vector modes in the path that is supposed to
handle shifts by a scalar. That looks odd. Also it should be easy
to promote/demote the scalar value to the proper type, so why not
do that? Like how we do
/* Unlike the other binary operators, shifts/rotates have
the rhs being int, instead of the same type as the lhs,
so make sure the scalar is the right type if we are
dealing with vectors of short/char. */
if (dt[1] == vect_constant_def)
op1 = fold_convert (TREE_TYPE (vectype), op1);
just do that for all kind of defs (and deal with the fact that you
may need a gimplified stmt for the vector shift amount generation).
?
Thanks,
Richard.
> 2011-10-27 Jakub Jelinek <jakub@redhat.com>
>
> * tree-vect-stmts.c (vectorizable_shift): Give up if op1 has different
> vector mode from vectype's mode.
>
> --- gcc/tree-vect-stmts.c.jj 2011-10-27 08:42:51.000000000 +0200
> +++ gcc/tree-vect-stmts.c 2011-10-27 17:24:15.000000000 +0200
> @@ -2318,6 +2318,7 @@ vectorizable_shift (gimple stmt, gimple_
> int nunits_in;
> int nunits_out;
> tree vectype_out;
> + tree op1_vectype;
> int ncopies;
> int j, i;
> VEC (tree, heap) *vec_oprnds0 = NULL, *vec_oprnds1 = NULL;
> @@ -2387,7 +2388,8 @@ vectorizable_shift (gimple stmt, gimple_
> return false;
>
> op1 = gimple_assign_rhs2 (stmt);
> - if (!vect_is_simple_use (op1, loop_vinfo, bb_vinfo, &def_stmt, &def, &dt[1]))
> + if (!vect_is_simple_use_1 (op1, loop_vinfo, bb_vinfo, &def_stmt, &def,
> + &dt[1], &op1_vectype))
> {
> if (vect_print_dump_info (REPORT_DETAILS))
> fprintf (vect_dump, "use not simple.");
> @@ -2444,6 +2446,13 @@ vectorizable_shift (gimple stmt, gimple_
> optab = optab_for_tree_code (code, vectype, optab_vector);
> if (vect_print_dump_info (REPORT_DETAILS))
> fprintf (vect_dump, "vector/vector shift/rotate found.");
> + if (TYPE_MODE (op1_vectype) != TYPE_MODE (vectype))
> + {
> + if (vect_print_dump_info (REPORT_DETAILS))
> + fprintf (vect_dump, "unusable type for last operand in"
> + " vector/vector shift/rotate.");
> + return false;
> + }
> }
> /* See if the machine has a vector shifted by scalar insn and if not
> then see if it has a vector shifted by vector insn. */
>
> Jakub
>
>
--
Richard Guenther <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Don't ICE on long long shifts in vectorizable_shift
2011-10-28 8:58 ` Richard Guenther
@ 2011-10-28 9:21 ` Jakub Jelinek
2011-10-28 10:40 ` Richard Guenther
0 siblings, 1 reply; 4+ messages in thread
From: Jakub Jelinek @ 2011-10-28 9:21 UTC (permalink / raw)
To: Richard Guenther; +Cc: Ira Rosen, gcc-patches
On Fri, Oct 28, 2011 at 10:22:15AM +0200, Richard Guenther wrote:
> Hm, but you are testing vector modes in the path that is supposed to
> handle shifts by a scalar. That looks odd. Also it should be easy
No, I'm testing it in the path that is supposed to handle shifts by a
vector. That block starts with:
/* Vector shifted by vector. */
if (!scalar_shift_arg)
{
Which means I'd have to duplicate there big parts of
vectorizable_type_promotion and vectorizable_type_demotion
and handle all these widening resp. narrowing cases.
I know you don't like tree-vect-pattern.c too much, but IMHO just
changing the shifts in there to have rhs2 type matching rhs1 type
would be far easier and more maintainable. Especially when it can handle
additionally what one of the testcases does - long long shift
with long long shift count, which should be naturally vectorized without
any promotion/demotion, but the FEs insert there a cast to (int) which would
result in the promotion/demotion.
Jakub
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Don't ICE on long long shifts in vectorizable_shift
2011-10-28 9:21 ` Jakub Jelinek
@ 2011-10-28 10:40 ` Richard Guenther
0 siblings, 0 replies; 4+ messages in thread
From: Richard Guenther @ 2011-10-28 10:40 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Ira Rosen, gcc-patches
On Fri, 28 Oct 2011, Jakub Jelinek wrote:
> On Fri, Oct 28, 2011 at 10:22:15AM +0200, Richard Guenther wrote:
> > Hm, but you are testing vector modes in the path that is supposed to
> > handle shifts by a scalar. That looks odd. Also it should be easy
>
> No, I'm testing it in the path that is supposed to handle shifts by a
> vector. That block starts with:
> /* Vector shifted by vector. */
> if (!scalar_shift_arg)
> {
Oh, I looked close and thought you are touching the else { part ...
> Which means I'd have to duplicate there big parts of
> vectorizable_type_promotion and vectorizable_type_demotion
> and handle all these widening resp. narrowing cases.
Yeah, agreed (though it should be always possible to just
truncate/extend the shift count).
> I know you don't like tree-vect-pattern.c too much, but IMHO just
> changing the shifts in there to have rhs2 type matching rhs1 type
> would be far easier and more maintainable. Especially when it can handle
> additionally what one of the testcases does - long long shift
> with long long shift count, which should be naturally vectorized without
> any promotion/demotion, but the FEs insert there a cast to (int) which would
> result in the promotion/demotion.
... but we could as well forward-prop this (also for scalar code)
(well, truncations only for SHIFT_COUNT_TRUNCATED targets).
The patch is ok meanwhile.
Thanks,
Richard.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-10-28 9:14 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-10-27 20:23 [PATCH] Don't ICE on long long shifts in vectorizable_shift Jakub Jelinek
2011-10-28 8:58 ` Richard Guenther
2011-10-28 9:21 ` Jakub Jelinek
2011-10-28 10:40 ` Richard Guenther
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).