| Message ID | 20220211112618.8961913C03@imap2.suse-dmz.suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | middle-end/104497 - gimplification of vector indexing | expand |
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; > +}
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; > > +} > >
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; +}