Re: [PATCH] middle-end/104497 - gimplification of vector indexing

[Jason Merrill <jason@redhat.com>]

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;
> +}