* [PATCH] middle-end/104497 - gimplification of vector indexing
@ 2022-02-11 11:26 Richard Biener
2022-02-11 17:36 ` Jason Merrill
0 siblings, 1 reply; 3+ messages in thread
From: Richard Biener @ 2022-02-11 11:26 UTC (permalink / raw)
To: gcc-patches
The following attempts to address gimplification of
... = VIEW_CONVERT_EXPR<int[4]>((i & 1) != 0 ? inv : src)[i];
which is problematic since gimplifying the base object
? inv : src produces a register temporary but GIMPLE does not
really support a register as a base for an ARRAY_REF (even
though that's not strictly validated it seems as can be seen
at -O0). Interestingly the C++ frontend avoids this issue
by emitting the following GENERIC instead:
... = (i & 1) != 0 ? VIEW_CONVERT_EXPR<int[4]>(inv)[i] : VIEW_CONVERT_EXPR<int[4]>(src)[i];
The proposed patch below fixes things up when using an rvalue
as the base is OK by emitting a copy from a register base to a
non-register one. The ?: as lvalue extension seems to be gone
for C, C++ again unwraps the COND_EXPR in that case.
Bootstrapped and tested on x86_64-unknown-linux-gnu.
OK?
Thanks,
Richard.
2022-02-11 Richard Biener <rguenther@suse.de>
PR middle-end/104497
* gimplify.cc (gimplify_compound_lval): Make sure the
base is a non-register if needed and possible.
* c-c++-common/torture/pr104497.c: New testcase.
---
gcc/gimplify.cc | 17 ++++++++++++++---
gcc/testsuite/c-c++-common/torture/pr104497.c | 12 ++++++++++++
2 files changed, 26 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/c-c++-common/torture/pr104497.c
diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
index 8d676fb96c8..cdf1ccbe48b 100644
--- a/gcc/gimplify.cc
+++ b/gcc/gimplify.cc
@@ -250,6 +250,7 @@ static enum gimplify_status gimplify_compound_expr (tree *, gimple_seq *, bool);
static hash_map<tree, tree> *oacc_declare_returns;
static enum gimplify_status gimplify_expr (tree *, gimple_seq *, gimple_seq *,
bool (*) (tree), fallback_t, bool);
+static void prepare_gimple_addressable (tree *, gimple_seq *);
/* Shorter alias name for the above function for use in gimplify.cc
only. */
@@ -3126,10 +3127,12 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
gimplified before gimplifying the size expressions.
So we do this in three steps. First we deal with variable
- bounds, sizes, and positions, then we gimplify the base,
- then we deal with the annotations for any variables in the
- components and any indices, from left to right. */
+ bounds, sizes, and positions, then we gimplify the base and
+ ensure it is memory if needed, then we deal with the annotations
+ for any variables in the components and any indices, from left
+ to right. */
+ bool need_non_reg = false;
for (i = expr_stack.length () - 1; i >= 0; i--)
{
tree t = expr_stack[i];
@@ -3165,6 +3168,7 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
TREE_OPERAND (t, 3) = elmt_size;
}
}
+ need_non_reg = true;
}
else if (TREE_CODE (t) == COMPONENT_REF)
{
@@ -3186,6 +3190,7 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
TREE_OPERAND (t, 2) = offset;
}
}
+ need_non_reg = true;
}
}
@@ -3196,6 +3201,12 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
fallback | fb_lvalue);
ret = MIN (ret, tret);
+ /* Step 2a: if we have component references we do not support on
+ registers then make sure the base isn't a register. Of course
+ we can only do so if an rvalue is OK. */
+ if (need_non_reg && (fallback & fb_rvalue))
+ prepare_gimple_addressable (p, pre_p);
+
/* Step 3: gimplify size expressions and the indices and operands of
ARRAY_REF. During this loop we also remove any useless conversions. */
diff --git a/gcc/testsuite/c-c++-common/torture/pr104497.c b/gcc/testsuite/c-c++-common/torture/pr104497.c
new file mode 100644
index 00000000000..c63fc021e03
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/torture/pr104497.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+
+typedef int __attribute__((vector_size(16))) vec_t;
+
+vec_t src, inv, res;
+
+void test(int i)
+{
+ vec_t y={0};
+ y[i] = (i & 1 ? inv : src)[i];
+ res = y;
+}
--
2.34.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] middle-end/104497 - gimplification of vector indexing
2022-02-11 11:26 [PATCH] middle-end/104497 - gimplification of vector indexing Richard Biener
@ 2022-02-11 17:36 ` Jason Merrill
2022-02-14 8:12 ` Richard Biener
0 siblings, 1 reply; 3+ messages in thread
From: Jason Merrill @ 2022-02-11 17:36 UTC (permalink / raw)
To: Richard Biener, gcc-patches
On 2/11/22 06:26, Richard Biener wrote:
> The following attempts to address gimplification of
>
> ... = VIEW_CONVERT_EXPR<int[4]>((i & 1) != 0 ? inv : src)[i];
>
> which is problematic since gimplifying the base object
> ? inv : src produces a register temporary but GIMPLE does not
> really support a register as a base for an ARRAY_REF (even
> though that's not strictly validated it seems as can be seen
> at -O0).
I suppose that isn't easy to fix?
And COMPONENT_REF has the same problem?
> Interestingly the C++ frontend avoids this issue
> by emitting the following GENERIC instead:
>
> ... = (i & 1) != 0 ? VIEW_CONVERT_EXPR<int[4]>(inv)[i] : VIEW_CONVERT_EXPR<int[4]>(src)[i];
Yes, because in C++ ?: of two lvalues is an lvalue.
> The proposed patch below fixes things up when using an rvalue
> as the base is OK by emitting a copy from a register base to a
> non-register one. The ?: as lvalue extension seems to be gone
> for C, C++ again unwraps the COND_EXPR in that case.
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu.
>
> OK?
OK, assuming "yes" answers to my questions above.
> Thanks,
> Richard.
>
> 2022-02-11 Richard Biener <rguenther@suse.de>
>
> PR middle-end/104497
> * gimplify.cc (gimplify_compound_lval): Make sure the
> base is a non-register if needed and possible.
>
> * c-c++-common/torture/pr104497.c: New testcase.
> ---
> gcc/gimplify.cc | 17 ++++++++++++++---
> gcc/testsuite/c-c++-common/torture/pr104497.c | 12 ++++++++++++
> 2 files changed, 26 insertions(+), 3 deletions(-)
> create mode 100644 gcc/testsuite/c-c++-common/torture/pr104497.c
>
> diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
> index 8d676fb96c8..cdf1ccbe48b 100644
> --- a/gcc/gimplify.cc
> +++ b/gcc/gimplify.cc
> @@ -250,6 +250,7 @@ static enum gimplify_status gimplify_compound_expr (tree *, gimple_seq *, bool);
> static hash_map<tree, tree> *oacc_declare_returns;
> static enum gimplify_status gimplify_expr (tree *, gimple_seq *, gimple_seq *,
> bool (*) (tree), fallback_t, bool);
> +static void prepare_gimple_addressable (tree *, gimple_seq *);
>
> /* Shorter alias name for the above function for use in gimplify.cc
> only. */
> @@ -3126,10 +3127,12 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
> gimplified before gimplifying the size expressions.
>
> So we do this in three steps. First we deal with variable
> - bounds, sizes, and positions, then we gimplify the base,
> - then we deal with the annotations for any variables in the
> - components and any indices, from left to right. */
> + bounds, sizes, and positions, then we gimplify the base and
> + ensure it is memory if needed, then we deal with the annotations
> + for any variables in the components and any indices, from left
> + to right. */
>
> + bool need_non_reg = false;
> for (i = expr_stack.length () - 1; i >= 0; i--)
> {
> tree t = expr_stack[i];
> @@ -3165,6 +3168,7 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
> TREE_OPERAND (t, 3) = elmt_size;
> }
> }
> + need_non_reg = true;
> }
> else if (TREE_CODE (t) == COMPONENT_REF)
> {
> @@ -3186,6 +3190,7 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
> TREE_OPERAND (t, 2) = offset;
> }
> }
> + need_non_reg = true;
> }
> }
>
> @@ -3196,6 +3201,12 @@ gimplify_compound_lval (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
> fallback | fb_lvalue);
> ret = MIN (ret, tret);
>
> + /* Step 2a: if we have component references we do not support on
> + registers then make sure the base isn't a register. Of course
> + we can only do so if an rvalue is OK. */
> + if (need_non_reg && (fallback & fb_rvalue))
> + prepare_gimple_addressable (p, pre_p);
> +
> /* Step 3: gimplify size expressions and the indices and operands of
> ARRAY_REF. During this loop we also remove any useless conversions. */
>
> diff --git a/gcc/testsuite/c-c++-common/torture/pr104497.c b/gcc/testsuite/c-c++-common/torture/pr104497.c
> new file mode 100644
> index 00000000000..c63fc021e03
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/torture/pr104497.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +
> +typedef int __attribute__((vector_size(16))) vec_t;
> +
> +vec_t src, inv, res;
> +
> +void test(int i)
> +{
> + vec_t y={0};
> + y[i] = (i & 1 ? inv : src)[i];
> + res = y;
> +}
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] middle-end/104497 - gimplification of vector indexing
2022-02-11 17:36 ` Jason Merrill
@ 2022-02-14 8:12 ` Richard Biener
0 siblings, 0 replies; 3+ messages in thread
From: Richard Biener @ 2022-02-14 8:12 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches
On Fri, 11 Feb 2022, Jason Merrill wrote:
> On 2/11/22 06:26, Richard Biener wrote:
> > The following attempts to address gimplification of
> >
> > ... = VIEW_CONVERT_EXPR<int[4]>((i & 1) != 0 ? inv : src)[i];
> >
> > which is problematic since gimplifying the base object
> > ? inv : src produces a register temporary but GIMPLE does not
> > really support a register as a base for an ARRAY_REF (even
> > though that's not strictly validated it seems as can be seen
> > at -O0).
>
> I suppose that isn't easy to fix?
I think it's more that we don't like to have that. There are some
optimization passes that do not expect SSA variables as bases
of (nested) tcc_reference ops. We obviously have a few exceptions
for non-nested {REAL,IMAG}PART_EXPR, BIT_FIELD_REF and
VIEW_CONVERT_EXRP. For the case in question it would be
1) ARRAY_REF, and 2) nested (because SSA names never have array type).
For IL "niceyness" iff we want a variable-index operation for
vector types I'd rather have non-nested tcc_reference here and
allow ARRAY_REF to operate on VECTOR_TYPE directly.
So yes, at this point that isn't easy to fix.
> And COMPONENT_REF has the same problem?
Yes. SSA names never have record or union type.
> > Interestingly the C++ frontend avoids this issue
> > by emitting the following GENERIC instead:
> >
> > ... = (i & 1) != 0 ? VIEW_CONVERT_EXPR<int[4]>(inv)[i] :
> > VIEW_CONVERT_EXPR<int[4]>(src)[i];
>
> Yes, because in C++ ?: of two lvalues is an lvalue.
Ah, so maybe one could reproduce with a mixed lvalue / rvalue.
No, that ends up with
(i & 1) != 0 ? VIEW_CONVERT_EXPR<int[4]>(inv)[i] : NON_LVALUE_EXPR
<VIEW_CONVERT_EXPR<int[4]>(NON_LVALUE_EXPR <src>)[i]>
then.
> > The proposed patch below fixes things up when using an rvalue
> > as the base is OK by emitting a copy from a register base to a
> > non-register one. The ?: as lvalue extension seems to be gone
> > for C, C++ again unwraps the COND_EXPR in that case.
> >
> > Bootstrapped and tested on x86_64-unknown-linux-gnu.
> >
> > OK?
>
> OK, assuming "yes" answers to my questions above.
So what eventually might work is have the C frontend produce IL
similat to the C++ FE. But then I'm not really sure that
the COND_EXPR case is the only one that requires special treatment.
It is really the gimplify_compound_lval outer refs speciality
that we do not communicate to the base gimplification, so
conceptually the fix is correct.
I've pushed the change now.
Thanks,
Richard.
> > Thanks,
> > Richard.
> >
> > 2022-02-11 Richard Biener <rguenther@suse.de>
> >
> > PR middle-end/104497
> > * gimplify.cc (gimplify_compound_lval): Make sure the
> > base is a non-register if needed and possible.
> >
> > * c-c++-common/torture/pr104497.c: New testcase.
> > ---
> > gcc/gimplify.cc | 17 ++++++++++++++---
> > gcc/testsuite/c-c++-common/torture/pr104497.c | 12 ++++++++++++
> > 2 files changed, 26 insertions(+), 3 deletions(-)
> > create mode 100644 gcc/testsuite/c-c++-common/torture/pr104497.c
> >
> > diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
> > index 8d676fb96c8..cdf1ccbe48b 100644
> > --- a/gcc/gimplify.cc
> > +++ b/gcc/gimplify.cc
> > @@ -250,6 +250,7 @@ static enum gimplify_status gimplify_compound_expr (tree
> > *, gimple_seq *, bool);
> > static hash_map<tree, tree> *oacc_declare_returns;
> > static enum gimplify_status gimplify_expr (tree *, gimple_seq *,
> > gimple_seq *,
> > bool (*) (tree), fallback_t, bool);
> > +static void prepare_gimple_addressable (tree *, gimple_seq *);
> >
> > /* Shorter alias name for the above function for use in gimplify.cc
> > only. */
> > @@ -3126,10 +3127,12 @@ gimplify_compound_lval (tree *expr_p, gimple_seq
> > *pre_p, gimple_seq *post_p,
> > gimplified before gimplifying the size expressions.
> >
> > So we do this in three steps. First we deal with variable
> > - bounds, sizes, and positions, then we gimplify the base,
> > - then we deal with the annotations for any variables in the
> > - components and any indices, from left to right. */
> > + bounds, sizes, and positions, then we gimplify the base and
> > + ensure it is memory if needed, then we deal with the annotations
> > + for any variables in the components and any indices, from left
> > + to right. */
> > + bool need_non_reg = false;
> > for (i = expr_stack.length () - 1; i >= 0; i--)
> > {
> > tree t = expr_stack[i];
> > @@ -3165,6 +3168,7 @@ gimplify_compound_lval (tree *expr_p, gimple_seq
> > *pre_p, gimple_seq *post_p,
> > TREE_OPERAND (t, 3) = elmt_size;
> > }
> > }
> > + need_non_reg = true;
> > }
> > else if (TREE_CODE (t) == COMPONENT_REF)
> > {
> > @@ -3186,6 +3190,7 @@ gimplify_compound_lval (tree *expr_p, gimple_seq
> > *pre_p, gimple_seq *post_p,
> > TREE_OPERAND (t, 2) = offset;
> > }
> > }
> > + need_non_reg = true;
> > }
> > }
> > @@ -3196,6 +3201,12 @@ gimplify_compound_lval (tree *expr_p, gimple_seq
> > *pre_p, gimple_seq *post_p,
> > fallback | fb_lvalue);
> > ret = MIN (ret, tret);
> > + /* Step 2a: if we have component references we do not support on
> > + registers then make sure the base isn't a register. Of course
> > + we can only do so if an rvalue is OK. */
> > + if (need_non_reg && (fallback & fb_rvalue))
> > + prepare_gimple_addressable (p, pre_p);
> > +
> > /* Step 3: gimplify size expressions and the indices and operands of
> > ARRAY_REF. During this loop we also remove any useless conversions.
> > */
> > diff --git a/gcc/testsuite/c-c++-common/torture/pr104497.c
> > b/gcc/testsuite/c-c++-common/torture/pr104497.c
> > new file mode 100644
> > index 00000000000..c63fc021e03
> > --- /dev/null
> > +++ b/gcc/testsuite/c-c++-common/torture/pr104497.c
> > @@ -0,0 +1,12 @@
> > +/* { dg-do compile } */
> > +
> > +typedef int __attribute__((vector_size(16))) vec_t;
> > +
> > +vec_t src, inv, res;
> > +
> > +void test(int i)
> > +{
> > + vec_t y={0};
> > + y[i] = (i & 1 ? inv : src)[i];
> > + res = y;
> > +}
>
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-02-14 8:12 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-11 11:26 [PATCH] middle-end/104497 - gimplification of vector indexing Richard Biener
2022-02-11 17:36 ` Jason Merrill
2022-02-14 8:12 ` Richard Biener
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).