* [PATCH] Create VECTOR_CST instead of CONSTRUCTORs where possible in the vectorizer (PR tree-optimization/33993)
@ 2007-11-05 12:13 Jakub Jelinek
2007-11-06 2:44 ` Mark Mitchell
2007-11-07 3:19 ` Dorit Nuzman
0 siblings, 2 replies; 6+ messages in thread
From: Jakub Jelinek @ 2007-11-05 12:13 UTC (permalink / raw)
To: gcc-patches
Hi!
On sparc64-linux gcc ICEs on the attached testcase with -O3, because stmt
verification doesn't like <VIEW_CONVERT_EXPR <CONSTRUCTOR <...>>, but
requires a CONSTANT_CLASS_P or is_gimple_lvalue inside of
handled_component_p. While that verification should be probably to allow
CONSTRUCTORs inside of VIEW_CONVERT_EXPR (guess no other
handled_component_p codes though), or at least some CONSTRUCTORs (which?
is_gimple_min_invariant ones, or only TREE_CONSTANT ones, only vector
ones or something else), this exact problem is because vectorizer is lazy
and creates CONSTRUCTORs even when VECTOR_CSTs should be used instead
(if these were gimplified, the gimplifier would change them to VECTOR_CSTs,
but they are not and also it would be wasteful to create CONSTRUCTORs only
to create VECTOR_CSTs from them immediately afterwards).
In vect_get_constant_vectors the parts can actually be sometimes constants,
sometimes something else, in the latter hunks I'm fairly certain only
constants can be seen among the parts - step_expr is an INTEGER_CST
(otherwise vect_is_simple_iv_evolution would return false and we'd fail
the assertion after it) and in some cases it is multiplied by another
INTEGER_CST.
Regtested on x86_64-linux, tested on the testcase on sparc64-linux.
Ok for trunk?
2007-11-05 Jakub Jelinek <jakub@redhat.com>
PR tree-optimization/33993
* tree-vect-transform.c (vect_get_constant_vectors): Use build_vector
rather than build_constructor_from_list if all list values are
constants.
(get_initial_def_for_induction): Use build_vector instead of
build_constructor_from_list.
* gcc.c-torture/compile/20071105-1.c: New test.
--- gcc/tree-vect-transform.c.jj 2007-11-05 09:05:44.000000000 +0100
+++ gcc/tree-vect-transform.c 2007-11-05 11:41:14.000000000 +0100
@@ -1318,6 +1318,7 @@ vect_get_constant_vectors (slp_tree slp_
bool is_store = false;
unsigned int number_of_vectors = SLP_TREE_NUMBER_OF_VEC_STMTS (slp_node);
VEC (tree, heap) *voprnds = VEC_alloc (tree, heap, number_of_vectors);
+ bool constant_p;
if (STMT_VINFO_DATA_REF (stmt_vinfo))
is_store = true;
@@ -1341,6 +1342,7 @@ vect_get_constant_vectors (slp_tree slp_
number_of_copies = least_common_multiple (nunits, group_size) / group_size;
number_of_places_left_in_vector = nunits;
+ constant_p = true;
for (j = 0; j < number_of_copies; j++)
{
for (i = group_size - 1; VEC_iterate (tree, stmts, i, stmt); i--)
@@ -1350,6 +1352,8 @@ vect_get_constant_vectors (slp_tree slp_
op = operation;
else
op = TREE_OPERAND (operation, op_num);
+ if (!CONSTANT_CLASS_P (op))
+ constant_p = false;
/* Create 'vect_ = {op0,op1,...,opn}'. */
t = tree_cons (NULL_TREE, op, t);
@@ -1362,7 +1366,11 @@ vect_get_constant_vectors (slp_tree slp_
vector_type = get_vectype_for_scalar_type (TREE_TYPE (op));
gcc_assert (vector_type);
- vec_cst = build_constructor_from_list (vector_type, t);
+ if (constant_p)
+ vec_cst = build_vector (vector_type, t);
+ else
+ vec_cst = build_constructor_from_list (vector_type, t);
+ constant_p = true;
VEC_quick_push (tree, voprnds,
vect_init_vector (stmt, vec_cst, vector_type,
NULL));
@@ -1617,7 +1625,8 @@ get_initial_def_for_induction (tree iv_p
t = NULL_TREE;
for (i = 0; i < nunits; i++)
t = tree_cons (NULL_TREE, unshare_expr (new_name), t);
- vec = build_constructor_from_list (vectype, t);
+ gcc_assert (CONSTANT_CLASS_P (new_name));
+ vec = build_vector (vectype, t);
vec_step = vect_init_vector (iv_phi, vec, vectype, NULL);
@@ -1673,7 +1682,8 @@ get_initial_def_for_induction (tree iv_p
t = NULL_TREE;
for (i = 0; i < nunits; i++)
t = tree_cons (NULL_TREE, unshare_expr (new_name), t);
- vec = build_constructor_from_list (vectype, t);
+ gcc_assert (CONSTANT_CLASS_P (new_name));
+ vec = build_vector (vectype, t);
vec_step = vect_init_vector (iv_phi, vec, vectype, NULL);
vec_def = induc_def;
--- gcc/testsuite/gcc.c-torture/compile/20071105-1.c.jj 2007-11-05 12:54:14.000000000 +0100
+++ gcc/testsuite/gcc.c-torture/compile/20071105-1.c 2007-11-05 12:53:47.000000000 +0100
@@ -0,0 +1,11 @@
+/* PR tree-optimization/33993 */
+/* Testcase by Martin Michlmayr <tbm@cyrius.com> */
+
+void
+init_full (char *array, int ny)
+{
+ int j;
+ char acc = 128;
+ for (j = 0; j < ny; j++)
+ *array++ = acc++;
+}
Jakub
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Create VECTOR_CST instead of CONSTRUCTORs where possible in the vectorizer (PR tree-optimization/33993)
2007-11-05 12:13 [PATCH] Create VECTOR_CST instead of CONSTRUCTORs where possible in the vectorizer (PR tree-optimization/33993) Jakub Jelinek
@ 2007-11-06 2:44 ` Mark Mitchell
2007-11-07 3:19 ` Dorit Nuzman
1 sibling, 0 replies; 6+ messages in thread
From: Mark Mitchell @ 2007-11-06 2:44 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
Jakub Jelinek wrote:
> 2007-11-05 Jakub Jelinek <jakub@redhat.com>
>
> PR tree-optimization/33993
> * tree-vect-transform.c (vect_get_constant_vectors): Use build_vector
> rather than build_constructor_from_list if all list values are
> constants.
> (get_initial_def_for_induction): Use build_vector instead of
> build_constructor_from_list.
>
> * gcc.c-torture/compile/20071105-1.c: New test.
OK.
--
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Create VECTOR_CST instead of CONSTRUCTORs where possible in the vectorizer (PR tree-optimization/33993)
2007-11-05 12:13 [PATCH] Create VECTOR_CST instead of CONSTRUCTORs where possible in the vectorizer (PR tree-optimization/33993) Jakub Jelinek
2007-11-06 2:44 ` Mark Mitchell
@ 2007-11-07 3:19 ` Dorit Nuzman
2007-11-07 16:37 ` Jakub Jelinek
1 sibling, 1 reply; 6+ messages in thread
From: Dorit Nuzman @ 2007-11-07 3:19 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
These appeared on powerpc64-linux since the commit of this patch (r129920):
FAIL: gcc.dg/vect/no-scevccp-outer-10a.c execution test
FAIL: gcc.dg/vect/no-scevccp-outer-10b.c execution test
FAIL: gcc.dg/vect/no-scevccp-outer-11.c execution test
FAIL: gcc.dg/vect/no-scevccp-outer-12.c execution test
FAIL: gcc.dg/vect/no-scevccp-outer-15.c execution test
FAIL: gcc.dg/vect/no-scevccp-outer-22.c execution test
FAIL: gcc.dg/vect/no-scevccp-outer-7.c execution test
FAIL: gcc.dg/vect/no-scevccp-noreassoc-outer-3.c execution test
dorit
> Hi!
>
> On sparc64-linux gcc ICEs on the attached testcase with -O3, because stmt
> verification doesn't like <VIEW_CONVERT_EXPR <CONSTRUCTOR <...>>, but
> requires a CONSTANT_CLASS_P or is_gimple_lvalue inside of
> handled_component_p. While that verification should be probably to allow
> CONSTRUCTORs inside of VIEW_CONVERT_EXPR (guess no other
> handled_component_p codes though), or at least some CONSTRUCTORs (which?
> is_gimple_min_invariant ones, or only TREE_CONSTANT ones, only vector
> ones or something else), this exact problem is because vectorizer is lazy
> and creates CONSTRUCTORs even when VECTOR_CSTs should be used instead
> (if these were gimplified, the gimplifier would change them to
VECTOR_CSTs,
> but they are not and also it would be wasteful to create CONSTRUCTORs
only
> to create VECTOR_CSTs from them immediately afterwards).
>
> In vect_get_constant_vectors the parts can actually be sometimes
constants,
> sometimes something else, in the latter hunks I'm fairly certain only
> constants can be seen among the parts - step_expr is an INTEGER_CST
> (otherwise vect_is_simple_iv_evolution would return false and we'd fail
> the assertion after it) and in some cases it is multiplied by another
> INTEGER_CST.
>
> Regtested on x86_64-linux, tested on the testcase on sparc64-linux.
> Ok for trunk?
>
> 2007-11-05 Jakub Jelinek <jakub@redhat.com>
>
> PR tree-optimization/33993
> * tree-vect-transform.c (vect_get_constant_vectors): Use build_vector
> rather than build_constructor_from_list if all list values are
> constants.
> (get_initial_def_for_induction): Use build_vector instead of
> build_constructor_from_list.
>
> * gcc.c-torture/compile/20071105-1.c: New test.
>
> --- gcc/tree-vect-transform.c.jj 2007-11-05 09:05:44.000000000 +0100
> +++ gcc/tree-vect-transform.c 2007-11-05 11:41:14.000000000 +0100
> @@ -1318,6 +1318,7 @@ vect_get_constant_vectors (slp_tree slp_
> bool is_store = false;
> unsigned int number_of_vectors = SLP_TREE_NUMBER_OF_VEC_STMTS
(slp_node);
> VEC (tree, heap) *voprnds = VEC_alloc (tree, heap, number_of_vectors);
> + bool constant_p;
>
> if (STMT_VINFO_DATA_REF (stmt_vinfo))
> is_store = true;
> @@ -1341,6 +1342,7 @@ vect_get_constant_vectors (slp_tree slp_
> number_of_copies = least_common_multiple (nunits, group_size) /
group_size;
>
> number_of_places_left_in_vector = nunits;
> + constant_p = true;
> for (j = 0; j < number_of_copies; j++)
> {
> for (i = group_size - 1; VEC_iterate (tree, stmts, i, stmt); i--)
> @@ -1350,6 +1352,8 @@ vect_get_constant_vectors (slp_tree slp_
> op = operation;
> else
> op = TREE_OPERAND (operation, op_num);
> + if (!CONSTANT_CLASS_P (op))
> + constant_p = false;
>
> /* Create 'vect_ = {op0,op1,...,opn}'. */
> t = tree_cons (NULL_TREE, op, t);
> @@ -1362,7 +1366,11 @@ vect_get_constant_vectors (slp_tree slp_
>
> vector_type = get_vectype_for_scalar_type (TREE_TYPE (op));
> gcc_assert (vector_type);
> - vec_cst = build_constructor_from_list (vector_type, t);
> + if (constant_p)
> + vec_cst = build_vector (vector_type, t);
> + else
> + vec_cst = build_constructor_from_list (vector_type, t);
> + constant_p = true;
> VEC_quick_push (tree, voprnds,
> vect_init_vector (stmt, vec_cst,
vector_type,
> NULL));
> @@ -1617,7 +1625,8 @@ get_initial_def_for_induction (tree iv_p
> t = NULL_TREE;
> for (i = 0; i < nunits; i++)
> t = tree_cons (NULL_TREE, unshare_expr (new_name), t);
> - vec = build_constructor_from_list (vectype, t);
> + gcc_assert (CONSTANT_CLASS_P (new_name));
> + vec = build_vector (vectype, t);
> vec_step = vect_init_vector (iv_phi, vec, vectype, NULL);
>
>
> @@ -1673,7 +1682,8 @@ get_initial_def_for_induction (tree iv_p
> t = NULL_TREE;
> for (i = 0; i < nunits; i++)
> t = tree_cons (NULL_TREE, unshare_expr (new_name), t);
> - vec = build_constructor_from_list (vectype, t);
> + gcc_assert (CONSTANT_CLASS_P (new_name));
> + vec = build_vector (vectype, t);
> vec_step = vect_init_vector (iv_phi, vec, vectype, NULL);
>
> vec_def = induc_def;
> --- gcc/testsuite/gcc.c-torture/compile/20071105-1.c.jj 2007-11-05
> 12:54:14.000000000 +0100
> +++ gcc/testsuite/gcc.c-torture/compile/20071105-1.c 2007-11-05
> 12:53:47.000000000 +0100
> @@ -0,0 +1,11 @@
> +/* PR tree-optimization/33993 */
> +/* Testcase by Martin Michlmayr <tbm@cyrius.com> */
> +
> +void
> +init_full (char *array, int ny)
> +{
> + int j;
> + char acc = 128;
> + for (j = 0; j < ny; j++)
> + *array++ = acc++;
> +}
>
> Jakub
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Create VECTOR_CST instead of CONSTRUCTORs where possible in the vectorizer (PR tree-optimization/33993)
2007-11-07 3:19 ` Dorit Nuzman
@ 2007-11-07 16:37 ` Jakub Jelinek
2007-11-07 17:01 ` Jakub Jelinek
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Jelinek @ 2007-11-07 16:37 UTC (permalink / raw)
To: Dorit Nuzman; +Cc: gcc-patches
On Tue, Nov 06, 2007 at 07:21:51PM -0800, Dorit Nuzman wrote:
> These appeared on powerpc64-linux since the commit of this patch (r129920):
>
> FAIL: gcc.dg/vect/no-scevccp-outer-10a.c execution test
> FAIL: gcc.dg/vect/no-scevccp-outer-10b.c execution test
> FAIL: gcc.dg/vect/no-scevccp-outer-11.c execution test
> FAIL: gcc.dg/vect/no-scevccp-outer-12.c execution test
> FAIL: gcc.dg/vect/no-scevccp-outer-15.c execution test
> FAIL: gcc.dg/vect/no-scevccp-outer-22.c execution test
> FAIL: gcc.dg/vect/no-scevccp-outer-7.c execution test
> FAIL: gcc.dg/vect/no-scevccp-noreassoc-outer-3.c execution test
Yeah, I can reproduce that, but it just seems to uncover a latent bug rather
than be a bug in the patch. In fact, if there wasn't this latent bug, with
the patch we generate far more efficient code, which is why I wonder if
the expander shouldn't check for VECTOR_TYPE CONSTRUCTORs with all entries
CONSTANT_CLASS_P and turn them into (or just expand them as) a VECTOR_CST.
From what I can see, gcc.dg/vect/no-scevccp-outer-10a.c is miscompiled
during sched2, where the scheduler happily moves set of v1 (and v12)
register accross a function call, in the former case it is not fatal on
Altivec CPUs (the vspltisw 1,4 insn is just moved before bl check_vector,
which means e.g. that on non-Altivec CPUs this will SIGILL), but in the
latter (vspltisw 12,4 insn moved in front of bl foo where it is used after
it) is fatal. Because:
if (TARGET_ALTIVEC_ABI)
{
for (i = FIRST_ALTIVEC_REGNO; i < FIRST_ALTIVEC_REGNO + 20; ++i)
call_used_regs[i] = call_really_used_regs[i] = 1;
and so both v1 and v12 are call used.
Jakub
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Create VECTOR_CST instead of CONSTRUCTORs where possible in the vectorizer (PR tree-optimization/33993)
2007-11-07 16:37 ` Jakub Jelinek
@ 2007-11-07 17:01 ` Jakub Jelinek
2007-11-07 17:05 ` Daniel Jacobowitz
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Jelinek @ 2007-11-07 17:01 UTC (permalink / raw)
To: Dorit Nuzman; +Cc: gcc-patches
On Wed, Nov 07, 2007 at 11:36:56AM -0500, Jakub Jelinek wrote:
> From what I can see, gcc.dg/vect/no-scevccp-outer-10a.c is miscompiled
> during sched2, where the scheduler happily moves set of v1 (and v12)
> register accross a function call, in the former case it is not fatal on
> Altivec CPUs (the vspltisw 1,4 insn is just moved before bl check_vector,
> which means e.g. that on non-Altivec CPUs this will SIGILL), but in the
> latter (vspltisw 12,4 insn moved in front of bl foo where it is used after
> it) is fatal. Because:
> if (TARGET_ALTIVEC_ABI)
> {
> for (i = FIRST_ALTIVEC_REGNO; i < FIRST_ALTIVEC_REGNO + 20; ++i)
> call_used_regs[i] = call_really_used_regs[i] = 1;
> and so both v1 and v12 are call used.
Actually, this is ppc64-linux -m32 -maltivec, no -mabi=altivec, so
these regs aren't call_used, but must be saved by foo when they are used.
But they don't seem to be saved/restored even when foo uses them...
Jakub
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Create VECTOR_CST instead of CONSTRUCTORs where possible in the vectorizer (PR tree-optimization/33993)
2007-11-07 17:01 ` Jakub Jelinek
@ 2007-11-07 17:05 ` Daniel Jacobowitz
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Jacobowitz @ 2007-11-07 17:05 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Dorit Nuzman, gcc-patches
On Wed, Nov 07, 2007 at 12:00:51PM -0500, Jakub Jelinek wrote:
> Actually, this is ppc64-linux -m32 -maltivec, no -mabi=altivec, so
> these regs aren't call_used, but must be saved by foo when they are used.
> But they don't seem to be saved/restored even when foo uses them...
I posted a patch for this a couple of weeks ago. Ulrich Weigand has
since suggested a better approach but I haven't had time to revise
yet.
http://sourceware.org/ml/gdb-patches/2007-10/msg00758.html
--
Daniel Jacobowitz
CodeSourcery
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2007-11-07 17:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-11-05 12:13 [PATCH] Create VECTOR_CST instead of CONSTRUCTORs where possible in the vectorizer (PR tree-optimization/33993) Jakub Jelinek
2007-11-06 2:44 ` Mark Mitchell
2007-11-07 3:19 ` Dorit Nuzman
2007-11-07 16:37 ` Jakub Jelinek
2007-11-07 17:01 ` Jakub Jelinek
2007-11-07 17:05 ` Daniel Jacobowitz
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).