Message ID | 20231102005737.2418307-1-juzhe.zhong@rivai.ai |
---|---|
State | New |
Headers | show |
Series | [tree-optimization/111721] VECT: Support SLP for MASK_LEN_GATHER_LOAD with dummy mask | expand |
On Thu, 2 Nov 2023, Juzhe-Zhong wrote: > This patch fixes following FAILs for RVV: > FAIL: gcc.dg/vect/vect-gather-1.c -flto -ffat-lto-objects scan-tree-dump vect "Loop contains only SLP stmts" > FAIL: gcc.dg/vect/vect-gather-1.c scan-tree-dump vect "Loop contains only SLP stmts" > > Bootstrap on X86 and regtest passed. > > Tested on aarch64 passed. > > Ok for trunk ? > > PR tree-optimization/111721 > > gcc/ChangeLog: > > * tree-vect-slp.cc (vect_get_and_check_slp_defs): Support SLP for dummy mask -1. > * tree-vect-stmts.cc (vectorizable_load): Ditto. > > --- > gcc/tree-vect-slp.cc | 14 ++++++++++++-- > gcc/tree-vect-stmts.cc | 8 +++++++- > 2 files changed, 19 insertions(+), 3 deletions(-) > > diff --git a/gcc/tree-vect-slp.cc b/gcc/tree-vect-slp.cc > index 43d742e3c92..23ca0318e31 100644 > --- a/gcc/tree-vect-slp.cc > +++ b/gcc/tree-vect-slp.cc > @@ -756,8 +756,7 @@ vect_get_and_check_slp_defs (vec_info *vinfo, unsigned char swap, > { > tree type = TREE_TYPE (oprnd); > dt = dts[i]; > - if ((dt == vect_constant_def > - || dt == vect_external_def) > + if (dt == vect_external_def > && !GET_MODE_SIZE (vinfo->vector_mode).is_constant () > && (TREE_CODE (type) == BOOLEAN_TYPE > || !can_duplicate_and_interleave_p (vinfo, stmts.length (), > @@ -769,6 +768,17 @@ vect_get_and_check_slp_defs (vec_info *vinfo, unsigned char swap, > "for variable-length SLP %T\n", oprnd); > return -1; > } > + if (dt == vect_constant_def > + && !GET_MODE_SIZE (vinfo->vector_mode).is_constant () > + && !can_duplicate_and_interleave_p (vinfo, stmts.length (), type)) > + { > + if (dump_enabled_p ()) > + dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location, > + "Build SLP failed: invalid type of def " > + "for variable-length SLP %T\n", > + oprnd); > + return -1; > + } I don't think that's quite correct. can_duplicate_and_interleave_p doesn't get enough info here and IIRC even materializing arbitrary constants isn't possible with VLA vectors. The very first thing the function does is tree base_vector_type = get_vectype_for_scalar_type (vinfo, elt_type, count); if (!base_vector_type || !VECTOR_MODE_P (TYPE_MODE (base_vector_type))) return false; but for masks that's not going to get us the correct vector type. While I don't understand why we have that 'BOOLEAN_TYPE' special case (maybe the intent was to identify 'mask' operands that way?), we might want to require that we can materialize both all-zero and all-ones constant 'mask's. But then 'mask' operands should be properly identified here. Maybe we can also simply delay the check to the point we know whether we're facing an uniform constant or not (note for 'first', we cannot really special-case vect_constant_def as the second SLP lane might demote that to vect_external_def). It's always a balance of whether to reject sth at SLP build time (possibly allowing operand swapping to do magic) or to delay checks to stmt analysis time. That might also explain that you do not see fallout of the "wrong" change (the later checking will catch it anyway). There's probably testsuite coverage for SVE here. That said, a "correct" patch might be to simply change && (TREE_CODE (type) == BOOLEAN_TYPE || !can_duplicate_and_interleave_p (vinfo, stmts.length (), type))) to && TREE_CODE (type) != BOOLEAN_TYPE && !can_duplicate_and_interleave_p (vinfo, stmts.length (), type) thus delay 'mask' operand validation here. Note I still think we should improve TREE_CODE (type) == BOOLEAN_TYPE to identify internal function mask operands only. Richard. > > /* For the swapping logic below force vect_reduction_def > for the reduction op in a SLP reduction group. */ > diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc > index 6ce4868d3e1..6c47121e158 100644 > --- a/gcc/tree-vect-stmts.cc > +++ b/gcc/tree-vect-stmts.cc > @@ -9859,10 +9859,16 @@ vectorizable_load (vec_info *vinfo, > mask_index = internal_fn_mask_index (ifn); > if (mask_index >= 0 && slp_node) > mask_index = vect_slp_child_index_for_operand (call, mask_index); > + slp_tree slp_op = NULL; > if (mask_index >= 0 > && !vect_check_scalar_mask (vinfo, stmt_info, slp_node, mask_index, > - &mask, NULL, &mask_dt, &mask_vectype)) > + &mask, &slp_op, &mask_dt, &mask_vectype)) > return false; > + /* MASK_LEN_GATHER_LOAD dummy mask -1 should always match the > + MASK_VECTYPE. */ > + if (mask_index >= 0 && slp_node && mask_dt == vect_constant_def > + && !vect_maybe_update_slp_op_vectype (slp_op, mask_vectype)) > + gcc_unreachable (); > } > > tree vectype = STMT_VINFO_VECTYPE (stmt_info); >
Thanks Richi. The following is the V2 patch: Testing on X86 and aarch64 are running. diff --git a/gcc/tree-vect-slp.cc b/gcc/tree-vect-slp.cc index 43d742e3c92..e7f7f976f11 100644 --- a/gcc/tree-vect-slp.cc +++ b/gcc/tree-vect-slp.cc @@ -760,7 +760,7 @@ vect_get_and_check_slp_defs (vec_info *vinfo, unsigned char swap, || dt == vect_external_def) && !GET_MODE_SIZE (vinfo->vector_mode).is_constant () && (TREE_CODE (type) == BOOLEAN_TYPE - || !can_duplicate_and_interleave_p (vinfo, stmts.length (), + && !can_duplicate_and_interleave_p (vinfo, stmts.length (), type))) { if (dump_enabled_p ()) diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc index 6ce4868d3e1..6c47121e158 100644 --- a/gcc/tree-vect-stmts.cc +++ b/gcc/tree-vect-stmts.cc @@ -9859,10 +9859,16 @@ vectorizable_load (vec_info *vinfo, mask_index = internal_fn_mask_index (ifn); if (mask_index >= 0 && slp_node) mask_index = vect_slp_child_index_for_operand (call, mask_index); + slp_tree slp_op = NULL; if (mask_index >= 0 && !vect_check_scalar_mask (vinfo, stmt_info, slp_node, mask_index, - &mask, NULL, &mask_dt, &mask_vectype)) + &mask, &slp_op, &mask_dt, &mask_vectype)) return false; + /* MASK_LEN_GATHER_LOAD dummy mask -1 should always match the + MASK_VECTYPE. */ + if (mask_index >= 0 && slp_node && mask_dt == vect_constant_def + && !vect_maybe_update_slp_op_vectype (slp_op, mask_vectype)) + gcc_unreachable (); } juzhe.zhong@rivai.ai From: Richard Biener Date: 2023-11-02 19:11 To: Juzhe-Zhong CC: gcc-patches; richard.sandiford Subject: Re: [tree-optimization/111721] VECT: Support SLP for MASK_LEN_GATHER_LOAD with dummy mask On Thu, 2 Nov 2023, Juzhe-Zhong wrote: > This patch fixes following FAILs for RVV: > FAIL: gcc.dg/vect/vect-gather-1.c -flto -ffat-lto-objects scan-tree-dump vect "Loop contains only SLP stmts" > FAIL: gcc.dg/vect/vect-gather-1.c scan-tree-dump vect "Loop contains only SLP stmts" > > Bootstrap on X86 and regtest passed. > > Tested on aarch64 passed. > > Ok for trunk ? > > PR tree-optimization/111721 > > gcc/ChangeLog: > > * tree-vect-slp.cc (vect_get_and_check_slp_defs): Support SLP for dummy mask -1. > * tree-vect-stmts.cc (vectorizable_load): Ditto. > > --- > gcc/tree-vect-slp.cc | 14 ++++++++++++-- > gcc/tree-vect-stmts.cc | 8 +++++++- > 2 files changed, 19 insertions(+), 3 deletions(-) > > diff --git a/gcc/tree-vect-slp.cc b/gcc/tree-vect-slp.cc > index 43d742e3c92..23ca0318e31 100644 > --- a/gcc/tree-vect-slp.cc > +++ b/gcc/tree-vect-slp.cc > @@ -756,8 +756,7 @@ vect_get_and_check_slp_defs (vec_info *vinfo, unsigned char swap, > { > tree type = TREE_TYPE (oprnd); > dt = dts[i]; > - if ((dt == vect_constant_def > - || dt == vect_external_def) > + if (dt == vect_external_def > && !GET_MODE_SIZE (vinfo->vector_mode).is_constant () > && (TREE_CODE (type) == BOOLEAN_TYPE > || !can_duplicate_and_interleave_p (vinfo, stmts.length (), > @@ -769,6 +768,17 @@ vect_get_and_check_slp_defs (vec_info *vinfo, unsigned char swap, > "for variable-length SLP %T\n", oprnd); > return -1; > } > + if (dt == vect_constant_def > + && !GET_MODE_SIZE (vinfo->vector_mode).is_constant () > + && !can_duplicate_and_interleave_p (vinfo, stmts.length (), type)) > + { > + if (dump_enabled_p ()) > + dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location, > + "Build SLP failed: invalid type of def " > + "for variable-length SLP %T\n", > + oprnd); > + return -1; > + } I don't think that's quite correct. can_duplicate_and_interleave_p doesn't get enough info here and IIRC even materializing arbitrary constants isn't possible with VLA vectors. The very first thing the function does is tree base_vector_type = get_vectype_for_scalar_type (vinfo, elt_type, count); if (!base_vector_type || !VECTOR_MODE_P (TYPE_MODE (base_vector_type))) return false; but for masks that's not going to get us the correct vector type. While I don't understand why we have that 'BOOLEAN_TYPE' special case (maybe the intent was to identify 'mask' operands that way?), we might want to require that we can materialize both all-zero and all-ones constant 'mask's. But then 'mask' operands should be properly identified here. Maybe we can also simply delay the check to the point we know whether we're facing an uniform constant or not (note for 'first', we cannot really special-case vect_constant_def as the second SLP lane might demote that to vect_external_def). It's always a balance of whether to reject sth at SLP build time (possibly allowing operand swapping to do magic) or to delay checks to stmt analysis time. That might also explain that you do not see fallout of the "wrong" change (the later checking will catch it anyway). There's probably testsuite coverage for SVE here. That said, a "correct" patch might be to simply change && (TREE_CODE (type) == BOOLEAN_TYPE || !can_duplicate_and_interleave_p (vinfo, stmts.length (), type))) to && TREE_CODE (type) != BOOLEAN_TYPE && !can_duplicate_and_interleave_p (vinfo, stmts.length (), type) thus delay 'mask' operand validation here. Note I still think we should improve TREE_CODE (type) == BOOLEAN_TYPE to identify internal function mask operands only. Richard. > > /* For the swapping logic below force vect_reduction_def > for the reduction op in a SLP reduction group. */ > diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc > index 6ce4868d3e1..6c47121e158 100644 > --- a/gcc/tree-vect-stmts.cc > +++ b/gcc/tree-vect-stmts.cc > @@ -9859,10 +9859,16 @@ vectorizable_load (vec_info *vinfo, > mask_index = internal_fn_mask_index (ifn); > if (mask_index >= 0 && slp_node) > mask_index = vect_slp_child_index_for_operand (call, mask_index); > + slp_tree slp_op = NULL; > if (mask_index >= 0 > && !vect_check_scalar_mask (vinfo, stmt_info, slp_node, mask_index, > - &mask, NULL, &mask_dt, &mask_vectype)) > + &mask, &slp_op, &mask_dt, &mask_vectype)) > return false; > + /* MASK_LEN_GATHER_LOAD dummy mask -1 should always match the > + MASK_VECTYPE. */ > + if (mask_index >= 0 && slp_node && mask_dt == vect_constant_def > + && !vect_maybe_update_slp_op_vectype (slp_op, mask_vectype)) > + gcc_unreachable (); > } > > tree vectype = STMT_VINFO_VECTYPE (stmt_info); >
On Thu, 2 Nov 2023, juzhe.zhong@rivai.ai wrote: > Thanks Richi. > > The following is the V2 patch: > Testing on X86 and aarch64 are running. > > diff --git a/gcc/tree-vect-slp.cc b/gcc/tree-vect-slp.cc > index 43d742e3c92..e7f7f976f11 100644 > --- a/gcc/tree-vect-slp.cc > +++ b/gcc/tree-vect-slp.cc > @@ -760,7 +760,7 @@ vect_get_and_check_slp_defs (vec_info *vinfo, unsigned char swap, > || dt == vect_external_def) > && !GET_MODE_SIZE (vinfo->vector_mode).is_constant () > && (TREE_CODE (type) == BOOLEAN_TYPE > - || !can_duplicate_and_interleave_p (vinfo, stmts.length (), > + && !can_duplicate_and_interleave_p (vinfo, stmts.length (), > type))) That's not what I wrote. I wrote to let == BOOLEAN_TYPE pass without check here, thus - && (TREE_CODE (type) == BOOLEAN_TYPE + && TREE_CODE (type) != BOOLEAN_TYPE && !can_duplicate... > { > if (dump_enabled_p ()) > diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc > index 6ce4868d3e1..6c47121e158 100644 > --- a/gcc/tree-vect-stmts.cc > +++ b/gcc/tree-vect-stmts.cc > @@ -9859,10 +9859,16 @@ vectorizable_load (vec_info *vinfo, > mask_index = internal_fn_mask_index (ifn); > if (mask_index >= 0 && slp_node) > mask_index = vect_slp_child_index_for_operand (call, mask_index); > + slp_tree slp_op = NULL; > if (mask_index >= 0 > && !vect_check_scalar_mask (vinfo, stmt_info, slp_node, mask_index, > - &mask, NULL, &mask_dt, &mask_vectype)) > + &mask, &slp_op, &mask_dt, &mask_vectype)) > return false; > + /* MASK_LEN_GATHER_LOAD dummy mask -1 should always match the > + MASK_VECTYPE. */ > + if (mask_index >= 0 && slp_node && mask_dt == vect_constant_def > + && !vect_maybe_update_slp_op_vectype (slp_op, mask_vectype)) > + gcc_unreachable (); You shouldn't do this here. Theres code in if (costing_p) that would need to be updated if you (correctly) want to track slp_op here. > } > > > > > juzhe.zhong@rivai.ai > > From: Richard Biener > Date: 2023-11-02 19:11 > To: Juzhe-Zhong > CC: gcc-patches; richard.sandiford > Subject: Re: [tree-optimization/111721] VECT: Support SLP for MASK_LEN_GATHER_LOAD with dummy mask > On Thu, 2 Nov 2023, Juzhe-Zhong wrote: > > > This patch fixes following FAILs for RVV: > > FAIL: gcc.dg/vect/vect-gather-1.c -flto -ffat-lto-objects scan-tree-dump vect "Loop contains only SLP stmts" > > FAIL: gcc.dg/vect/vect-gather-1.c scan-tree-dump vect "Loop contains only SLP stmts" > > > > Bootstrap on X86 and regtest passed. > > > > Tested on aarch64 passed. > > > > Ok for trunk ? > > > > PR tree-optimization/111721 > > > > gcc/ChangeLog: > > > > * tree-vect-slp.cc (vect_get_and_check_slp_defs): Support SLP for dummy mask -1. > > * tree-vect-stmts.cc (vectorizable_load): Ditto. > > > > --- > > gcc/tree-vect-slp.cc | 14 ++++++++++++-- > > gcc/tree-vect-stmts.cc | 8 +++++++- > > 2 files changed, 19 insertions(+), 3 deletions(-) > > > > diff --git a/gcc/tree-vect-slp.cc b/gcc/tree-vect-slp.cc > > index 43d742e3c92..23ca0318e31 100644 > > --- a/gcc/tree-vect-slp.cc > > +++ b/gcc/tree-vect-slp.cc > > @@ -756,8 +756,7 @@ vect_get_and_check_slp_defs (vec_info *vinfo, unsigned char swap, > > { > > tree type = TREE_TYPE (oprnd); > > dt = dts[i]; > > - if ((dt == vect_constant_def > > - || dt == vect_external_def) > > + if (dt == vect_external_def > > && !GET_MODE_SIZE (vinfo->vector_mode).is_constant () > > && (TREE_CODE (type) == BOOLEAN_TYPE > > || !can_duplicate_and_interleave_p (vinfo, stmts.length (), > > @@ -769,6 +768,17 @@ vect_get_and_check_slp_defs (vec_info *vinfo, unsigned char swap, > > "for variable-length SLP %T\n", oprnd); > > return -1; > > } > > + if (dt == vect_constant_def > > + && !GET_MODE_SIZE (vinfo->vector_mode).is_constant () > > + && !can_duplicate_and_interleave_p (vinfo, stmts.length (), type)) > > + { > > + if (dump_enabled_p ()) > > + dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location, > > + "Build SLP failed: invalid type of def " > > + "for variable-length SLP %T\n", > > + oprnd); > > + return -1; > > + } > > I don't think that's quite correct. can_duplicate_and_interleave_p > doesn't get enough info here and IIRC even materializing arbitrary > constants isn't possible with VLA vectors. The very first thing > the function does is > > tree base_vector_type = get_vectype_for_scalar_type (vinfo, elt_type, > count); > if (!base_vector_type || !VECTOR_MODE_P (TYPE_MODE (base_vector_type))) > return false; > > but for masks that's not going to get us the correct vector type. > While I don't understand why we have that 'BOOLEAN_TYPE' special > case (maybe the intent was to identify 'mask' operands that way?), > we might want to require that we can materialize both all-zero > and all-ones constant 'mask's. But then 'mask' operands should > be properly identified here. > > Maybe we can also simply delay the check to the point we know > whether we're facing an uniform constant or not (note for 'first', > we cannot really special-case vect_constant_def as the second > SLP lane might demote that to vect_external_def). It's always > a balance of whether to reject sth at SLP build time (possibly > allowing operand swapping to do magic) or to delay checks > to stmt analysis time. That might also explain that you > do not see fallout of the "wrong" change (the later checking > will catch it anyway). > > There's probably testsuite coverage for SVE here. > > That said, a "correct" patch might be to simply change > > && (TREE_CODE (type) == BOOLEAN_TYPE > || !can_duplicate_and_interleave_p (vinfo, stmts.length > (), > type))) > > to > > && TREE_CODE (type) != BOOLEAN_TYPE > && !can_duplicate_and_interleave_p (vinfo, stmts.length > (), > type) > > thus delay 'mask' operand validation here. > > Note I still think we should improve TREE_CODE (type) == BOOLEAN_TYPE > to identify internal function mask operands only. > > Richard. > > > > > > /* For the swapping logic below force vect_reduction_def > > for the reduction op in a SLP reduction group. */ > > diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc > > index 6ce4868d3e1..6c47121e158 100644 > > --- a/gcc/tree-vect-stmts.cc > > +++ b/gcc/tree-vect-stmts.cc > > @@ -9859,10 +9859,16 @@ vectorizable_load (vec_info *vinfo, > > mask_index = internal_fn_mask_index (ifn); > > if (mask_index >= 0 && slp_node) > > mask_index = vect_slp_child_index_for_operand (call, mask_index); > > + slp_tree slp_op = NULL; > > if (mask_index >= 0 > > && !vect_check_scalar_mask (vinfo, stmt_info, slp_node, mask_index, > > - &mask, NULL, &mask_dt, &mask_vectype)) > > + &mask, &slp_op, &mask_dt, &mask_vectype)) > > return false; > > + /* MASK_LEN_GATHER_LOAD dummy mask -1 should always match the > > + MASK_VECTYPE. */ > > + if (mask_index >= 0 && slp_node && mask_dt == vect_constant_def > > + && !vect_maybe_update_slp_op_vectype (slp_op, mask_vectype)) > > + gcc_unreachable (); > > } > > > > tree vectype = STMT_VINFO_VECTYPE (stmt_info); > > > >
diff --git a/gcc/tree-vect-slp.cc b/gcc/tree-vect-slp.cc index 43d742e3c92..23ca0318e31 100644 --- a/gcc/tree-vect-slp.cc +++ b/gcc/tree-vect-slp.cc @@ -756,8 +756,7 @@ vect_get_and_check_slp_defs (vec_info *vinfo, unsigned char swap, { tree type = TREE_TYPE (oprnd); dt = dts[i]; - if ((dt == vect_constant_def - || dt == vect_external_def) + if (dt == vect_external_def && !GET_MODE_SIZE (vinfo->vector_mode).is_constant () && (TREE_CODE (type) == BOOLEAN_TYPE || !can_duplicate_and_interleave_p (vinfo, stmts.length (), @@ -769,6 +768,17 @@ vect_get_and_check_slp_defs (vec_info *vinfo, unsigned char swap, "for variable-length SLP %T\n", oprnd); return -1; } + if (dt == vect_constant_def + && !GET_MODE_SIZE (vinfo->vector_mode).is_constant () + && !can_duplicate_and_interleave_p (vinfo, stmts.length (), type)) + { + if (dump_enabled_p ()) + dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location, + "Build SLP failed: invalid type of def " + "for variable-length SLP %T\n", + oprnd); + return -1; + } /* For the swapping logic below force vect_reduction_def for the reduction op in a SLP reduction group. */ diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc index 6ce4868d3e1..6c47121e158 100644 --- a/gcc/tree-vect-stmts.cc +++ b/gcc/tree-vect-stmts.cc @@ -9859,10 +9859,16 @@ vectorizable_load (vec_info *vinfo, mask_index = internal_fn_mask_index (ifn); if (mask_index >= 0 && slp_node) mask_index = vect_slp_child_index_for_operand (call, mask_index); + slp_tree slp_op = NULL; if (mask_index >= 0 && !vect_check_scalar_mask (vinfo, stmt_info, slp_node, mask_index, - &mask, NULL, &mask_dt, &mask_vectype)) + &mask, &slp_op, &mask_dt, &mask_vectype)) return false; + /* MASK_LEN_GATHER_LOAD dummy mask -1 should always match the + MASK_VECTYPE. */ + if (mask_index >= 0 && slp_node && mask_dt == vect_constant_def + && !vect_maybe_update_slp_op_vectype (slp_op, mask_vectype)) + gcc_unreachable (); } tree vectype = STMT_VINFO_VECTYPE (stmt_info);