Richard Biener <rguent...@suse.de> writes:
> This allows vectorization (in practice non-loop vectorization) to
> have a stmt participate in different vector type vectorizations.
> It allows us to remove vect_update_shared_vectype and replace it
> by pushing/popping STMT_VINFO_VECTYPE from SLP_TREE_VECTYPE around
> vect_analyze_stmt and vect_transform_stmt.
>
> For data-ref the situation is a bit more complicated since we
> analyze alignment info with a specific vector type in mind which
> doesn't play well when that changes.
>
> So the bulk of the change is passing down the actual vector type
> used for a vectorized access to the various accessors of alignment
> info, first and foremost dr_misalignment but also aligned_access_p,
> known_alignment_for_access_p, vect_known_alignment_in_bytes and
> vect_supportable_dr_alignment. I took the liberty to replace
> ALL_CAPS macro accessors with the lower-case function invocations.
>
> The actual changes to the behavior are in dr_misalignment which now
> is the place factoring in the negative step adjustment as well as
> handling alignment queries for a vector type with bigger alignment
> requirements than what we can (or have) analyze(d).
>
> vect_slp_analyze_node_alignment makes use of this and upon receiving
> a vector type with a bigger alingment desire re-analyzes the DR
> with respect to it but keeps an older more precise result if possible.
> In this context it might be possible to do the analysis just once
> but instead of analyzing with respect to a specific desired alignment
> look for the biggest alignment we can compute a not unknown alignment.
>
> The ChangeLog includes the functional changes but not the bulk due
> to the alignment accessor API changes - I hope that's something good.
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu, testing on SPEC
> CPU 2017 in progress (for stats and correctness).
>
> Any comments?
Sorry for the super-slow response, some comments below.
> […]
> diff --git a/gcc/tree-vect-data-refs.c b/gcc/tree-vect-data-refs.c
> index a57700f2c1b..c42fc2fb272 100644
> --- a/gcc/tree-vect-data-refs.c
> +++ b/gcc/tree-vect-data-refs.c
> @@ -887,37 +887,53 @@ vect_slp_analyze_instance_dependence (vec_info *vinfo,
> slp_instance instance)
> return res;
> }
>
> -/* Return the misalignment of DR_INFO. */
> +/* Return the misalignment of DR_INFO accessed in VECTYPE. */
>
> int
> -dr_misalignment (dr_vec_info *dr_info)
> +dr_misalignment (dr_vec_info *dr_info, tree vectype)
> {
> + HOST_WIDE_INT diff = 0;
> + /* Alignment is only analyzed for the first element of a DR group,
> + use that but adjust misalignment by the offset of the access. */
> if (STMT_VINFO_GROUPED_ACCESS (dr_info->stmt))
> {
> dr_vec_info *first_dr
> = STMT_VINFO_DR_INFO (DR_GROUP_FIRST_ELEMENT (dr_info->stmt));
> - int misalign = first_dr->misalignment;
> - gcc_assert (misalign != DR_MISALIGNMENT_UNINITIALIZED);
> - if (misalign == DR_MISALIGNMENT_UNKNOWN)
> - return misalign;
> /* vect_analyze_data_ref_accesses guarantees that DR_INIT are
> INTEGER_CSTs and the first element in the group has the lowest
> address. Likewise vect_compute_data_ref_alignment will
> have ensured that target_alignment is constant and otherwise
> set misalign to DR_MISALIGNMENT_UNKNOWN. */
Can you move the second sentence down so that it stays with the to_constant?
> - HOST_WIDE_INT diff = (TREE_INT_CST_LOW (DR_INIT (dr_info->dr))
> - - TREE_INT_CST_LOW (DR_INIT (first_dr->dr)));
> + diff = (TREE_INT_CST_LOW (DR_INIT (dr_info->dr))
> + - TREE_INT_CST_LOW (DR_INIT (first_dr->dr)));
> gcc_assert (diff >= 0);
> - unsigned HOST_WIDE_INT target_alignment_c
> - = first_dr->target_alignment.to_constant ();
> - return (misalign + diff) % target_alignment_c;
> + dr_info = first_dr;
> }
> - else
> +
> + int misalign = dr_info->misalignment;
> + gcc_assert (misalign != DR_MISALIGNMENT_UNINITIALIZED);
> + if (misalign == DR_MISALIGNMENT_UNKNOWN)
> + return misalign;
> +
> + /* If the access is only aligned for a vector type with smaller alignment
> + requirement the access has unknown misalignment. */
> + if (maybe_lt (dr_info->target_alignment * BITS_PER_UNIT,
> + targetm.vectorize.preferred_vector_alignment (vectype)))
> + return DR_MISALIGNMENT_UNKNOWN;
> +
> + /* If this is a backward running DR then first access in the larger
> + vectype actually is N-1 elements before the address in the DR.
> + Adjust misalign accordingly. */
> + if (tree_int_cst_sgn (DR_STEP (dr_info->dr)) < 0)
> {
> - int misalign = dr_info->misalignment;
> - gcc_assert (misalign != DR_MISALIGNMENT_UNINITIALIZED);
> - return misalign;
> + if (!TYPE_VECTOR_SUBPARTS (vectype).is_constant ())
> + return DR_MISALIGNMENT_UNKNOWN;
> + diff += ((TYPE_VECTOR_SUBPARTS (vectype).to_constant () - 1)
> + * -TREE_INT_CST_LOW (TYPE_SIZE_UNIT (TREE_TYPE (vectype))));
The target alignment might only be element alignment. If so, I think we
should either skip this block or return early above. This would avoid
returning DR_MISALIGNMENT_UNKNOWN unnecessarily for VLA.
Or, more generally, I think this is case where known_misalignment
does work. I.e. diff can be a poly_int64 and:
> }
> + unsigned HOST_WIDE_INT target_alignment_c
> + = dr_info->target_alignment.to_constant ();
> + return (misalign + diff) % target_alignment_c;
the last line can become:
if (!known_misalignment (misalign + diff, target_alignment_c, &misalign))
return DR_MISALIGNMENT_UNKNOWN;
return misalign;
avoiding the need for the is_constant above.
> […]
> @@ -1896,10 +1902,9 @@ vect_enhance_data_refs_alignment (loop_vec_info
> loop_vinfo)
> unsigned int target_align =
> DR_TARGET_ALIGNMENT (dr_info).to_constant ();
> unsigned int dr_size = vect_get_scalar_dr_size (dr_info);
> - mis = (negative
> - ? DR_MISALIGNMENT (dr_info)
> - : -DR_MISALIGNMENT (dr_info));
> - if (DR_MISALIGNMENT (dr_info) != 0)
> + unsigned int mis = dr_misalignment (dr_info, vectype);
> + mis = negative ? mis : -mis;
Just checking: is this still correct? It probably is, but it looked a
bit like we're handling negative steps twice. Same further down.
LGTM otherwise FWIW.
Thanks,
Richard
