Re: [PATCH v4 1/3] IFN: Implement IFN_VEC_SET for ARRAY_REF with VIEW_CONVERT_EXPR
On 2020/9/25 21:28, Richard Sandiford wrote:
> xionghu luo writes:
>> @@ -2658,6 +2659,45 @@ expand_vect_cond_mask_optab_fn (internal_fn, gcall
>> *stmt, convert_optab optab)
>>
>> #define expand_vec_cond_mask_optab_fn expand_vect_cond_mask_optab_fn
>>
>> +/* Expand VEC_SET internal functions. */
>> +
>> +static void
>> +expand_vec_set_optab_fn (internal_fn, gcall *stmt, convert_optab optab)
>> +{
>> + tree lhs = gimple_call_lhs (stmt);
>> + tree op0 = gimple_call_arg (stmt, 0);
>> + tree op1 = gimple_call_arg (stmt, 1);
>> + tree op2 = gimple_call_arg (stmt, 2);
>> + rtx target = expand_expr (lhs, NULL_RTX, VOIDmode, EXPAND_WRITE);
>> + rtx src = expand_normal (op0);
>> +
>> + machine_mode outermode = TYPE_MODE (TREE_TYPE (op0));
>> + scalar_mode innermode = GET_MODE_INNER (outermode);
>> +
>> + rtx value = expand_expr (op1, NULL_RTX, VOIDmode, EXPAND_NORMAL);
>> + rtx pos = expand_expr (op2, NULL_RTX, VOIDmode, EXPAND_NORMAL);
>
> These two can just use expand_normal. Might be easier to read if
> they come immediately after the expand_normal (op0).
>
> LGTM with that change for the internal-fn.c stuff, thanks.
>
Thank you, updated and committed as r11-3486. Tested and confirmed
Power/X86/ARM
still not supporting vec_set with register index, so there are no ICE
regressions
caused by generating IFN VEC_SET but not properly expanded.
Thanks,
Xionghu
Re: [PATCH v4 1/3] IFN: Implement IFN_VEC_SET for ARRAY_REF with VIEW_CONVERT_EXPR
xionghu luo writes:
> @@ -2658,6 +2659,45 @@ expand_vect_cond_mask_optab_fn (internal_fn, gcall
> *stmt, convert_optab optab)
>
> #define expand_vec_cond_mask_optab_fn expand_vect_cond_mask_optab_fn
>
> +/* Expand VEC_SET internal functions. */
> +
> +static void
> +expand_vec_set_optab_fn (internal_fn, gcall *stmt, convert_optab optab)
> +{
> + tree lhs = gimple_call_lhs (stmt);
> + tree op0 = gimple_call_arg (stmt, 0);
> + tree op1 = gimple_call_arg (stmt, 1);
> + tree op2 = gimple_call_arg (stmt, 2);
> + rtx target = expand_expr (lhs, NULL_RTX, VOIDmode, EXPAND_WRITE);
> + rtx src = expand_normal (op0);
> +
> + machine_mode outermode = TYPE_MODE (TREE_TYPE (op0));
> + scalar_mode innermode = GET_MODE_INNER (outermode);
> +
> + rtx value = expand_expr (op1, NULL_RTX, VOIDmode, EXPAND_NORMAL);
> + rtx pos = expand_expr (op2, NULL_RTX, VOIDmode, EXPAND_NORMAL);
These two can just use expand_normal. Might be easier to read if
they come immediately after the expand_normal (op0).
LGTM with that change for the internal-fn.c stuff, thanks.
Richard
Re: [PATCH v4 1/3] IFN: Implement IFN_VEC_SET for ARRAY_REF with VIEW_CONVERT_EXPR
On Fri, Sep 25, 2020 at 8:51 AM xionghu luo wrote:
>
> Hi,
>
> On 2020/9/24 20:39, Richard Sandiford wrote:
> > xionghu luo writes:
> >> @@ -2658,6 +2659,43 @@ expand_vect_cond_mask_optab_fn (internal_fn, gcall
> >> *stmt, convert_optab optab)
> >>
> >> #define expand_vec_cond_mask_optab_fn expand_vect_cond_mask_optab_fn
> >>
> >> +/* Expand VEC_SET internal functions. */
> >> +
> >> +static void
> >> +expand_vec_set_optab_fn (internal_fn, gcall *stmt, convert_optab optab)
> >> +{
> >> + tree lhs = gimple_call_lhs (stmt);
> >> + tree op0 = gimple_call_arg (stmt, 0);
> >> + tree op1 = gimple_call_arg (stmt, 1);
> >> + tree op2 = gimple_call_arg (stmt, 2);
> >> + rtx target = expand_expr (lhs, NULL_RTX, VOIDmode, EXPAND_WRITE);
> >> + rtx src = expand_expr (op0, NULL_RTX, VOIDmode, EXPAND_WRITE);
> >
> > I'm not sure about the expand_expr here. ISTM that op0 is a normal
> > input and so should be expanded by expand_normal rather than
> > EXPAND_WRITE. Also:
> >
> >> +
> >> + machine_mode outermode = TYPE_MODE (TREE_TYPE (op0));
> >> + scalar_mode innermode = GET_MODE_INNER (outermode);
> >> +
> >> + rtx value = expand_expr (op1, NULL_RTX, VOIDmode, EXPAND_NORMAL);
> >> + rtx pos = expand_expr (op2, NULL_RTX, VOIDmode, EXPAND_NORMAL);
> >> +
> >> + class expand_operand ops[3];
> >> + enum insn_code icode = optab_handler (optab, outermode);
> >> +
> >> + if (icode != CODE_FOR_nothing)
> >> +{
> >> + pos = convert_to_mode (E_SImode, pos, 0);
> >> +
> >> + create_fixed_operand ([0], src);
> >
> > ...this would mean that if SRC happens to be a MEM, the pattern
> > must also accept a MEM.
> >
> > ISTM that we're making more work for ourselves by not “fixing” the optab
> > to have a natural pure-input + pure-output interface. :-) But if we
> > stick with the current optab interface, I think we need to:
> >
> > - create a temporary register
> > - move SRC into the temporary register before the insn
> > - use create_fixed_operand with the temporary register for operand 0
> > - move the temporary register into TARGET after the insn
> >
> >> + create_input_operand ([1], value, innermode);
> >> + create_input_operand ([2], pos, GET_MODE (pos));
> >
> > For this I think we should use convert_operand_from on the original “pos”,
> > so that the target gets to choose what the mode of the operand is.
> >
>
> Thanks a lot for the nice suggestions, fixed them all and updated the patch
> as below.
>
>
> [PATCH v4 1/3] IFN: Implement IFN_VEC_SET for ARRAY_REF with VIEW_CONVERT_EXPR
>
> This patch enables transformation from ARRAY_REF(VIEW_CONVERT_EXPR) to
> VEC_SET internal function in gimple-isel pass if target supports
> vec_set with variable index by checking can_vec_set_var_idx_p.
OK with me if Richard is happy with the updated patch.
Thanks,
Richard.
> gcc/ChangeLog:
>
> 2020-09-25 Xionghu Luo
>
> * gimple-isel.cc (gimple_expand_vec_set_expr): New function.
> (gimple_expand_vec_cond_exprs): Rename to ...
> (gimple_expand_vec_exprs): ... this and call
> gimple_expand_vec_set_expr.
> * internal-fn.c (vec_set_direct): New define.
> (expand_vec_set_optab_fn): New function.
> (direct_vec_set_optab_supported_p): New define.
> * internal-fn.def (VEC_SET): New DEF_INTERNAL_OPTAB_FN.
> * optabs.c (can_vec_set_var_idx_p): New function.
> * optabs.h (can_vec_set_var_idx_p): New declaration.
> ---
> gcc/gimple-isel.cc | 75 +++--
> gcc/internal-fn.c | 41 +
> gcc/internal-fn.def | 2 ++
> gcc/optabs.c| 21 +
> gcc/optabs.h| 4 +++
> 5 files changed, 141 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/gimple-isel.cc b/gcc/gimple-isel.cc
> index b330cf4c20e..02513e04900 100644
> --- a/gcc/gimple-isel.cc
> +++ b/gcc/gimple-isel.cc
> @@ -35,6 +35,74 @@ along with GCC; see the file COPYING3. If not see
> #include "tree-cfg.h"
> #include "bitmap.h"
> #include "tree-ssa-dce.h"
> +#include "memmodel.h"
> +#include "optabs.h"
> +
> +/* Expand all ARRAY_REF(VIEW_CONVERT_EXPR) gimple assignments into calls to
> + internal function based on vector type of selected expansion.
> + i.e.:
> + VIEW_CONVERT_EXPR(u)[_1] = = i_4(D);
> + =>
> + _7 = u;
> +
[PATCH v4 1/3] IFN: Implement IFN_VEC_SET for ARRAY_REF with VIEW_CONVERT_EXPR
Hi,
On 2020/9/24 20:39, Richard Sandiford wrote:
> xionghu luo writes:
>> @@ -2658,6 +2659,43 @@ expand_vect_cond_mask_optab_fn (internal_fn, gcall
>> *stmt, convert_optab optab)
>>
>> #define expand_vec_cond_mask_optab_fn expand_vect_cond_mask_optab_fn
>>
>> +/* Expand VEC_SET internal functions. */
>> +
>> +static void
>> +expand_vec_set_optab_fn (internal_fn, gcall *stmt, convert_optab optab)
>> +{
>> + tree lhs = gimple_call_lhs (stmt);
>> + tree op0 = gimple_call_arg (stmt, 0);
>> + tree op1 = gimple_call_arg (stmt, 1);
>> + tree op2 = gimple_call_arg (stmt, 2);
>> + rtx target = expand_expr (lhs, NULL_RTX, VOIDmode, EXPAND_WRITE);
>> + rtx src = expand_expr (op0, NULL_RTX, VOIDmode, EXPAND_WRITE);
>
> I'm not sure about the expand_expr here. ISTM that op0 is a normal
> input and so should be expanded by expand_normal rather than
> EXPAND_WRITE. Also:
>
>> +
>> + machine_mode outermode = TYPE_MODE (TREE_TYPE (op0));
>> + scalar_mode innermode = GET_MODE_INNER (outermode);
>> +
>> + rtx value = expand_expr (op1, NULL_RTX, VOIDmode, EXPAND_NORMAL);
>> + rtx pos = expand_expr (op2, NULL_RTX, VOIDmode, EXPAND_NORMAL);
>> +
>> + class expand_operand ops[3];
>> + enum insn_code icode = optab_handler (optab, outermode);
>> +
>> + if (icode != CODE_FOR_nothing)
>> +{
>> + pos = convert_to_mode (E_SImode, pos, 0);
>> +
>> + create_fixed_operand ([0], src);
>
> ...this would mean that if SRC happens to be a MEM, the pattern
> must also accept a MEM.
>
> ISTM that we're making more work for ourselves by not “fixing” the optab
> to have a natural pure-input + pure-output interface. :-) But if we
> stick with the current optab interface, I think we need to:
>
> - create a temporary register
> - move SRC into the temporary register before the insn
> - use create_fixed_operand with the temporary register for operand 0
> - move the temporary register into TARGET after the insn
>
>> + create_input_operand ([1], value, innermode);
>> + create_input_operand ([2], pos, GET_MODE (pos));
>
> For this I think we should use convert_operand_from on the original “pos”,
> so that the target gets to choose what the mode of the operand is.
>
Thanks a lot for the nice suggestions, fixed them all and updated the patch as
below.
[PATCH v4 1/3] IFN: Implement IFN_VEC_SET for ARRAY_REF with VIEW_CONVERT_EXPR
This patch enables transformation from ARRAY_REF(VIEW_CONVERT_EXPR) to
VEC_SET internal function in gimple-isel pass if target supports
vec_set with variable index by checking can_vec_set_var_idx_p.
gcc/ChangeLog:
2020-09-25 Xionghu Luo
* gimple-isel.cc (gimple_expand_vec_set_expr): New function.
(gimple_expand_vec_cond_exprs): Rename to ...
(gimple_expand_vec_exprs): ... this and call
gimple_expand_vec_set_expr.
* internal-fn.c (vec_set_direct): New define.
(expand_vec_set_optab_fn): New function.
(direct_vec_set_optab_supported_p): New define.
* internal-fn.def (VEC_SET): New DEF_INTERNAL_OPTAB_FN.
* optabs.c (can_vec_set_var_idx_p): New function.
* optabs.h (can_vec_set_var_idx_p): New declaration.
---
gcc/gimple-isel.cc | 75 +++--
gcc/internal-fn.c | 41 +
gcc/internal-fn.def | 2 ++
gcc/optabs.c| 21 +
gcc/optabs.h| 4 +++
5 files changed, 141 insertions(+), 2 deletions(-)
diff --git a/gcc/gimple-isel.cc b/gcc/gimple-isel.cc
index b330cf4c20e..02513e04900 100644
--- a/gcc/gimple-isel.cc
+++ b/gcc/gimple-isel.cc
@@ -35,6 +35,74 @@ along with GCC; see the file COPYING3. If not see
#include "tree-cfg.h"
#include "bitmap.h"
#include "tree-ssa-dce.h"
+#include "memmodel.h"
+#include "optabs.h"
+
+/* Expand all ARRAY_REF(VIEW_CONVERT_EXPR) gimple assignments into calls to
+ internal function based on vector type of selected expansion.
+ i.e.:
+ VIEW_CONVERT_EXPR(u)[_1] = = i_4(D);
+ =>
+ _7 = u;
+ _8 = .VEC_SET (_7, i_4(D), _1);
+ u = _8; */
+
+static gimple *
+gimple_expand_vec_set_expr (gimple_stmt_iterator *gsi)
+{
+ enum tree_code code;
+ gcall *new_stmt = NULL;
+ gassign *ass_stmt = NULL;
+
+ /* Only consider code == GIMPLE_ASSIGN. */
+ gassign *stmt = dyn_cast (gsi_stmt (*gsi));
+ if (!stmt)
+return NULL;
+
+ tree lhs = gimple_assign_lhs (stmt);
+ code = TREE_CODE (lhs);
+ if (code != ARRAY_REF)
+return NULL;
+
+ tree val = gimple_assign_rhs1 (stmt);
+ tree op0 = TREE_OPERAND (lhs, 0);
+ if (TREE_CODE (op0) == VIEW_CONVERT_EXPR && DECL_P (TREE_OPERAND (op0, 0))
+ && VECTOR_TYPE_P (TREE_TYPE (TR
