On Tue, Sep 22, 2020 at 5:55 AM xionghu luo <luo...@linux.ibm.com> wrote:
>
> Thanks for the review,
>
>
> On 2020/9/21 16:31, Richard Biener wrote:
> >> +
> >> +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<gassign *> (gsi_stmt (*gsi));
> >> +  if (!stmt)
> >> +    return NULL;
> >> +
> >> +  code = TREE_CODE (gimple_assign_lhs (stmt));
> >
> > do the lhs = gimple_assign_lhs (stmt) before and elide cond,
> > putting the TREE_CODE into the if below.
>
> Done.
>
> >
> >> +  if (code != ARRAY_REF)
> >> +    return NULL;
> >> +
> >> +  tree lhs = gimple_assign_lhs (stmt);
> >> +  tree val = gimple_assign_rhs1 (stmt);
> >> +
> >> +  tree type = TREE_TYPE (lhs);
> >> +  tree op0 = TREE_OPERAND (lhs, 0);
> >> +  if (TREE_CODE (op0) == VIEW_CONVERT_EXPR
> >
> > So I think we want to have an exact structural match first here, so
> >
> >    if (TREE_CODE (op0) == VIEW_CONVERT_EXPR
> >        && DECL_P (TREE_OPERAND (op0, 0))
> >        && VECTOR_TYPE_P (TREE_TYPE (TREE_OPERAND (op0, 0)))
> >        && TYPE_MODE (TREE_TYPE (lhs)) == TYPE_MODE (TREE_TYPE
> > (TREE_TYPE (TREE_OPERAND (op0, 0))))
> >
> > which means we're sure to do an element extract from a vector type
> > (and we know all vector types have sane element types).
>
> Done.
>
> >
> >
> >> +      && tree_fits_uhwi_p (TYPE_SIZE (type)))
> >> +    {
> >> +      tree pos = TREE_OPERAND (lhs, 1);
> >> +      tree view_op0 = TREE_OPERAND (op0, 0);
> >> +      machine_mode outermode = TYPE_MODE (TREE_TYPE (view_op0));
> >> +      scalar_mode innermode = GET_MODE_INNER (outermode);
> >> +      tree_code code = TREE_CODE (TREE_TYPE(view_op0));
> >> +      if (!is_global_var (view_op0) && code == VECTOR_TYPE
> >> +         && tree_fits_uhwi_p (TYPE_SIZE (TREE_TYPE (view_op0)))
> >
> > why did you need those TYPE_SIZE checks?  As said earlier
> > you want !TREE_ADDRESSABLE (view_op0) and eventually
> > the stronger auto_var_in_fn_p (view_op0, cfun) rather than !is_global_var.
>
> Done.
>
> >
> >> +         && can_vec_set_var_idx_p (code, outermode, innermode,
> >> +                                   TYPE_MODE (TREE_TYPE (pos))))
> >> +       {
> >> +         location_t loc = gimple_location (stmt);
> >> +         tree var_src = make_ssa_name (TREE_TYPE (view_op0));
> >> +         tree var_dst = make_ssa_name (TREE_TYPE (view_op0));
> >> +
> >> +         ass_stmt = gimple_build_assign (var_src, view_op0);
> >> +         gimple_set_vuse (ass_stmt, gimple_vuse (stmt));
> >> +         gimple_set_location (ass_stmt, loc);
> >> +         gsi_insert_before (gsi, ass_stmt, GSI_SAME_STMT);
> >> +
> >> +         new_stmt
> >> +           = gimple_build_call_internal (IFN_VEC_SET, 3, var_src, val, 
> >> pos);
> >> +         gimple_call_set_lhs (new_stmt, var_dst);
> >> +         gimple_set_location (new_stmt, loc);
> >> +         gsi_insert_before (gsi, new_stmt, GSI_SAME_STMT);
> >> +
> >> +         ass_stmt = gimple_build_assign (view_op0, var_dst);
> >> +         gimple_set_location (ass_stmt, loc);
> >> +         gsi_insert_before (gsi, ass_stmt, GSI_SAME_STMT);
> >> +
> >> +         gimple_move_vops (ass_stmt, stmt);
> >> +         gsi_remove (gsi, true);
> >> +       }
> >> +    }
> >> +
> >> +  return ass_stmt;
> >> +}
> >>
> >>   /* Expand all VEC_COND_EXPR gimple assignments into calls to internal
> >>      function based on type of selected expansion.  */
> >> @@ -187,8 +261,25 @@ gimple_expand_vec_cond_exprs (void)
> >>       {
> >>         for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> >>          {
> >> -         gimple *g = gimple_expand_vec_cond_expr (&gsi,
> >> -                                                  
> >> &vec_cond_ssa_name_uses);
> >> +         gassign *stmt = dyn_cast<gassign *> (gsi_stmt (gsi));
> >> +         if (!stmt)
> >> +           continue;
> >> +
> >> +         enum tree_code code;
> >> +         gimple *g = NULL;
> >> +         code = gimple_assign_rhs_code (stmt);
> >> +         switch (code)
> >> +           {
> >> +           case VEC_COND_EXPR:
> >> +             g = gimple_expand_vec_cond_expr (&gsi, 
> >> &vec_cond_ssa_name_uses);
> >> +             break;
> >> +           case ARRAY_REF:
> >> +             /*  TODO: generate IFN for vec_extract with variable index.  
> >> */
> >
> > so why not do this here?
> >
> >> +             break;
> >> +           default:
> >> +             break;
> >> +           }
> >> +
> >>            if (g != NULL)
> >>              {
> >>                tree lhs = gimple_assign_lhs (gsi_stmt (gsi));
> >> @@ -204,6 +295,27 @@ gimple_expand_vec_cond_exprs (void)
> >>
> >>     simple_dce_from_worklist (dce_ssa_names);
> >>
> >> +  FOR_EACH_BB_FN (bb, cfun)
> >
> > but in a separate loop?
>
> The first loop is for rhs stmt process, this loop is for lhs stmt process.
> I thought vec_extract also need to generate IFN before, but seems not
> necessary now?  And that the first loop needs to update the lhs stmt while
> then second doesn't.

That's not good reasons to separate them, please move all the processing
into one loop.

+         gassign *stmt = dyn_cast<gassign *> (gsi_stmt (gsi));
+         if (!stmt)
+           continue;
+
+         enum tree_code code;
+         code = TREE_CODE (gimple_assign_lhs (stmt));
+         switch (code)
+           {
+           case ARRAY_REF:
+             gimple_expand_vec_set_expr (&gsi);

you also do the assign and ARRAY_REF checking duplicate.

The patch likely wasn't bootstrapped because I've seen unused and
set-but-not-used
variables.

Otherwise the patch looks good to me - I guess you want to add the
vec_extract bits as well so you can overall assess the affect of the patch
on altivec code?  That said, the patch misses a testcase where we verify
we properly expand the vector to a pseudo now.

Richard.

> >
> >> +    {
> >> +      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> >> +       {
> >> +         gassign *stmt = dyn_cast<gassign *> (gsi_stmt (gsi));
> >> +         if (!stmt)
> >> +           continue;
> >> +
> >> +         enum tree_code code;
> >> +         code = TREE_CODE (gimple_assign_lhs (stmt));
> >> +         switch (code)
> >> +           {
> >> +           case ARRAY_REF:
> >> +             gimple_expand_vec_set_expr (&gsi);
> >> +             break;
> >> +           default:
> >> +             break;
> >> +           }
> >> +       }
> >> +    }
> >> +
> >>     return 0;
> >>   }
> >>
> >> diff --git a/gcc/internal-fn.c b/gcc/internal-fn.c
> >> index 8efc77d986b..36837381c04 100644
> >> --- a/gcc/internal-fn.c
> >> +++ b/gcc/internal-fn.c
> >> @@ -115,6 +115,7 @@ init_internal_fns ()
> >>   #define vec_condeq_direct { 0, 0, false }
> >>   #define scatter_store_direct { 3, 1, false }
> >>   #define len_store_direct { 3, 3, false }
> >> +#define vec_set_direct { 3, 3, false }
> >>   #define unary_direct { 0, 0, true }
> >>   #define binary_direct { 0, 0, true }
> >>   #define ternary_direct { 0, 0, true }
> >> @@ -2658,6 +2659,40 @@ 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
> >>
> >> +static void
> >> +expand_vec_set_optab_fn (internal_fn, gcall *stmt, convert_optab optab)
> >
> > all new functions require a function level comment
>
> Done.
>
> >
> >> +{
> >> +  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);
> >> +
> >> +  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 (&ops[0], src);
> >> +      create_input_operand (&ops[1], value, innermode);
> >> +      create_input_operand (&ops[2], pos, GET_MODE (pos));
> >> +      if (maybe_expand_insn (icode, 3, ops))
> >> +       {
> >> +         emit_move_insn (target, src);
> >
> > I think you need to assert that we end up here.
>
> Added gcc_unreachable at the end of this function.
>
> >
> >> +         return;
> >> +       }
> >> +    }
> >> +}
> >> +
> >>   static void
> >>   expand_ABNORMAL_DISPATCHER (internal_fn, gcall *)
> >>   {
> >> @@ -3253,6 +3288,7 @@ multi_vector_optab_supported_p (convert_optab optab, 
> >> tree_pair types,
> >>   #define direct_fold_left_optab_supported_p direct_optab_supported_p
> >>   #define direct_mask_fold_left_optab_supported_p direct_optab_supported_p
> >>   #define direct_check_ptrs_optab_supported_p direct_optab_supported_p
> >> +#define direct_vec_set_optab_supported_p direct_optab_supported_p
> >>
> >>   /* Return the optab used by internal function FN.  */
> >>
> >> diff --git a/gcc/internal-fn.def b/gcc/internal-fn.def
> >> index 13e60828fcf..e6cfe1b6159 100644
> >> --- a/gcc/internal-fn.def
> >> +++ b/gcc/internal-fn.def
> >> @@ -145,6 +145,8 @@ DEF_INTERNAL_OPTAB_FN (VCONDU, 0, vcondu, vec_condu)
> >>   DEF_INTERNAL_OPTAB_FN (VCONDEQ, 0, vcondeq, vec_condeq)
> >>   DEF_INTERNAL_OPTAB_FN (VCOND_MASK, 0, vcond_mask, vec_cond_mask)
> >>
> >> +DEF_INTERNAL_OPTAB_FN (VEC_SET, 0, vec_set, vec_set)
> >> +
> >>   DEF_INTERNAL_OPTAB_FN (LEN_STORE, 0, len_store, len_store)
> >>
> >>   DEF_INTERNAL_OPTAB_FN (WHILE_ULT, ECF_CONST | ECF_NOTHROW, while_ult, 
> >> while)
> >> diff --git a/gcc/optabs.c b/gcc/optabs.c
> >> index 184827fdf4e..c8125670d2d 100644
> >> --- a/gcc/optabs.c
> >> +++ b/gcc/optabs.c
> >> @@ -3841,6 +3841,23 @@ can_vcond_compare_p (enum rtx_code code, 
> >> machine_mode value_mode,
> >>           && insn_operand_matches (icode, 3, test);
> >>   }
> >>
> >> +bool
> >> +can_vec_set_var_idx_p (enum tree_code code, machine_mode vec_mode,
> >> +                      machine_mode value_mode, machine_mode idx_mode)
> >
> > toplevel comment missing
> >
> >> +{
> >> +  gcc_assert (code == VECTOR_TYPE);
> >
> > what's the point of pasing 'code' here then?  Since the optab only has a 
> > single
> > mode, the vector mode, the value_mode is redundant as well.  And I guess
> > we might want to handle "arbitrary" index modes?  That is, the .md expanders
> > should not restrict its mode - I guess it simply uses VOIDmode at the moment
> > (for integer constants).  Not sure how to best do this without an explicit 
> > mode
> > in the optab ...
>
> Yes, removed 'code' and value_mode by checking VECTOR_MODE_P and use 
> GET_MODE_INNER
> for value_mode.  ".md expanders" shall support for integer constants index 
> mode, but
> I guess they shouldn't be expanded by IFN as this function is for variable 
> index
> insert only?  Anyway, the v3 patch used VOIDmode check...
>
>
> Thanks,
> Xionghu
>

Reply via email to