This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [7/7] Pool alignment information for common bases
On Thu, Jul 27, 2017 at 2:50 PM, Richard Sandiford
<richard.sandiford@linaro.org> wrote:
> Richard Biener <richard.guenther@gmail.com> writes:
>> On Tue, Jul 4, 2017 at 2:01 PM, Richard Sandiford
>> <richard.sandiford@linaro.org> wrote:
>>> Richard Biener <richard.guenther@gmail.com> writes:
>>>> On Mon, Jul 3, 2017 at 9:49 AM, Richard Sandiford
>>>> <richard.sandiford@linaro.org> wrote:
>>>>> @@ -2070,8 +2143,7 @@ vect_find_same_alignment_drs (struct dat
>>>>> if (dra == drb)
>>>>> return;
>>>>>
>>>>> - if (!operand_equal_p (DR_BASE_OBJECT (dra), DR_BASE_OBJECT (drb),
>>>>> - OEP_ADDRESS_OF)
>>>>> + if (!operand_equal_p (DR_BASE_ADDRESS (dra), DR_BASE_ADDRESS (drb), 0)
>>>>
>>>> Why this change? It's semantically weaker after your change.
>>>
>>> It's because the DR_BASE_OBJECT comes from the access_fn analysis
>>> while the DR_BASE_ADDRESS comes from the innermost_loop_behavior.
>>> I hadn't realised when adding the original code how different the
>>> two were, and since all the other parts are based on the
>>> innermost_loop_behavior, I think this check should be too.
>>> E.g. it doesn't really make sense to compare DR_INITs based
>>> on DR_BASE_OBJECT.
>>
>> Ah ok, makes sense now.
>>
>>> I guess it should have been a separate patch though.
>>
>> No need.
>>
>>>>> || !operand_equal_p (DR_OFFSET (dra), DR_OFFSET (drb), 0)
>>>>> || !operand_equal_p (DR_STEP (dra), DR_STEP (drb), 0))
>>>>> return;
>>>>> @@ -2129,6 +2201,7 @@ vect_analyze_data_refs_alignment (loop_v
>>>>> vec<data_reference_p> datarefs = vinfo->datarefs;
>>>>> struct data_reference *dr;
>>>>>
>>>>> + vect_record_base_alignments (vinfo);
>>>>> FOR_EACH_VEC_ELT (datarefs, i, dr)
>>>>> {
>>>>> stmt_vec_info stmt_info = vinfo_for_stmt (DR_STMT (dr));
>>>>> @@ -3327,7 +3400,8 @@ vect_analyze_data_refs (vec_info *vinfo,
>>>>> {
>>>>> struct data_reference *newdr
>>>>> = create_data_ref (NULL, loop_containing_stmt (stmt),
>>>>> - DR_REF (dr), stmt, maybe_scatter ? false : true);
>>>>> + DR_REF (dr), stmt, !maybe_scatter,
>>>>> + DR_IS_CONDITIONAL_IN_STMT (dr));
>>>>> gcc_assert (newdr != NULL && DR_REF (newdr));
>>>>> if (DR_BASE_ADDRESS (newdr)
>>>>> && DR_OFFSET (newdr)
>>>>> Index: gcc/tree-vect-slp.c
>>>>> ===================================================================
>>>>> --- gcc/tree-vect-slp.c 2017-07-03 08:20:56.404763323 +0100
>>>>> +++ gcc/tree-vect-slp.c 2017-07-03 08:42:51.149380545 +0100
>>>>> @@ -2358,6 +2358,7 @@ new_bb_vec_info (gimple_stmt_iterator re
>>>>> gimple_stmt_iterator gsi;
>>>>>
>>>>> res = (bb_vec_info) xcalloc (1, sizeof (struct _bb_vec_info));
>>>>> + new (&res->base_alignments) vec_base_alignments ();
>>>>
>>>> Ick. I'd rather make this proper C++ and do
>>>>
>>>> res = new _bb_vec_info;
>>>>
>>>> and add a constructor to the vec_info base initializing the hashtable.
>>>> The above smells fishy.
>>>
>>> I knew I pushing my luck with that one. I just didn't want to have to
>>> convert the current xcalloc of loop_vec_info into a long list of explicit
>>> zero initializers. (OK, we have a lot of explicit zero assignments already,
>>> but certainly some fields rely on the calloc zeroing.)
>>>
>>>> Looks like vec<> are happy with .create () being called on a zeroed struct.
>>>>
>>>> Alternatively add .create () to hashtable/map.
>>>
>>> The difference is that vec<> is explicitly meant to be POD, whereas
>>> hash_map isn't (and I don't think we want it to be).
>>
>> Ah, indeed. So your patch makes *vec_info no longer POD (no problem
>> but then use new/delete and constructors).
>>
>>> Ah well. I'll do a separate pre-patch to C++-ify the structures.
>
> Here's an update that applies on top of the vec_info c++-ification patch
> I just posted [ https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01807.html ]
>
> Tested as before. OK to install?
Ok.
Thanks,
Richard.
> Thanks,
> Richard
>
>
> 2017-07-27 Richard Sandiford <richard.sandiford@linaro.org>
>
> gcc/
> PR tree-optimization/81136
> * tree-vectorizer.h: Include tree-hash-traits.h.
> (vec_base_alignments): New typedef.
> (vec_info): Add a base_alignments field.
> (vect_record_base_alignments): Declare.
> * tree-data-ref.h (data_reference): Add an is_conditional_in_stmt
> field.
> (DR_IS_CONDITIONAL_IN_STMT): New macro.
> (create_data_ref): Add an is_conditional_in_stmt argument.
> * tree-data-ref.c (create_data_ref): Likewise. Use it to initialize
> the is_conditional_in_stmt field.
> (data_ref_loc): Add an is_conditional_in_stmt field.
> (get_references_in_stmt): Set the is_conditional_in_stmt field.
> (find_data_references_in_stmt): Update call to create_data_ref.
> (graphite_find_data_references_in_stmt): Likewise.
> * tree-ssa-loop-prefetch.c (determine_loop_nest_reuse): Likewise.
> * tree-vect-data-refs.c (vect_analyze_data_refs): Likewise.
> (vect_record_base_alignment): New function.
> (vect_record_base_alignments): Likewise.
> (vect_compute_data_ref_alignment): Adjust base_addr and aligned_to
> for nested statements even if we fail to compute a misalignment.
> Use pooled base alignments for unconditional references.
> (vect_find_same_alignment_drs): Compare base addresses instead
> of base objects.
> (vect_analyze_data_refs_alignment): Call vect_record_base_alignments.
> * tree-vect-slp.c (vect_slp_analyze_bb_1): Likewise.
>
> gcc/testsuite/
> PR tree-optimization/81136
> * gcc.dg/vect/pr81136.c: Add scan test.
>
> Index: gcc/tree-vectorizer.h
> ===================================================================
> --- gcc/tree-vectorizer.h 2017-07-27 13:46:58.376642483 +0100
> +++ gcc/tree-vectorizer.h 2017-07-27 13:46:58.579633332 +0100
> @@ -22,6 +22,7 @@ Software Foundation; either version 3, o
> #define GCC_TREE_VECTORIZER_H
>
> #include "tree-data-ref.h"
> +#include "tree-hash-traits.h"
> #include "target.h"
>
> /* Used for naming of new temporaries. */
> @@ -84,6 +85,11 @@ struct stmt_info_for_cost {
>
> typedef vec<stmt_info_for_cost> stmt_vector_for_cost;
>
> +/* Maps base addresses to an innermost_loop_behavior that gives the maximum
> + known alignment for that base. */
> +typedef hash_map<tree_operand_hash,
> + innermost_loop_behavior *> vec_base_alignments;
> +
> /************************************************************************
> SLP
> ************************************************************************/
> @@ -169,6 +175,10 @@ struct vec_info {
> /* All data references. Freed by free_data_refs, so not an auto_vec. */
> vec<data_reference_p> datarefs;
>
> + /* Maps base addresses to an innermost_loop_behavior that gives the maximum
> + known alignment for that base. */
> + vec_base_alignments base_alignments;
> +
> /* All data dependences. Freed by free_dependence_relations, so not
> an auto_vec. */
> vec<ddr_p> ddrs;
> @@ -1162,6 +1172,7 @@ extern bool vect_prune_runtime_alias_tes
> extern bool vect_check_gather_scatter (gimple *, loop_vec_info,
> gather_scatter_info *);
> extern bool vect_analyze_data_refs (vec_info *, int *);
> +extern void vect_record_base_alignments (vec_info *);
> extern tree vect_create_data_ref_ptr (gimple *, tree, struct loop *, tree,
> tree *, gimple_stmt_iterator *,
> gimple **, bool, bool *,
> Index: gcc/tree-data-ref.h
> ===================================================================
> --- gcc/tree-data-ref.h 2017-07-27 13:46:58.376642483 +0100
> +++ gcc/tree-data-ref.h 2017-07-27 13:46:58.578633377 +0100
> @@ -159,6 +159,11 @@ struct data_reference
> /* True when the data reference is in RHS of a stmt. */
> bool is_read;
>
> + /* True when the data reference is conditional within STMT,
> + i.e. if it might not occur even when the statement is executed
> + and runs to completion. */
> + bool is_conditional_in_stmt;
> +
> /* Behavior of the memory reference in the innermost loop. */
> struct innermost_loop_behavior innermost;
>
> @@ -178,6 +183,7 @@ #define DR_ACCESS_FN(DR, I) DR_AC
> #define DR_NUM_DIMENSIONS(DR) DR_ACCESS_FNS (DR).length ()
> #define DR_IS_READ(DR) (DR)->is_read
> #define DR_IS_WRITE(DR) (!DR_IS_READ (DR))
> +#define DR_IS_CONDITIONAL_IN_STMT(DR) (DR)->is_conditional_in_stmt
> #define DR_BASE_ADDRESS(DR) (DR)->innermost.base_address
> #define DR_OFFSET(DR) (DR)->innermost.offset
> #define DR_INIT(DR) (DR)->innermost.init
> @@ -434,7 +440,8 @@ extern bool graphite_find_data_reference
> vec<data_reference_p> *);
> tree find_data_references_in_loop (struct loop *, vec<data_reference_p> *);
> bool loop_nest_has_data_refs (loop_p loop);
> -struct data_reference *create_data_ref (loop_p, loop_p, tree, gimple *, bool);
> +struct data_reference *create_data_ref (loop_p, loop_p, tree, gimple *, bool,
> + bool);
> extern bool find_loop_nest (struct loop *, vec<loop_p> *);
> extern struct data_dependence_relation *initialize_data_dependence_relation
> (struct data_reference *, struct data_reference *, vec<loop_p>);
> Index: gcc/tree-data-ref.c
> ===================================================================
> --- gcc/tree-data-ref.c 2017-07-27 13:46:58.376642483 +0100
> +++ gcc/tree-data-ref.c 2017-07-27 13:46:58.577633422 +0100
> @@ -1125,15 +1125,19 @@ free_data_ref (data_reference_p dr)
> free (dr);
> }
>
> -/* Analyzes memory reference MEMREF accessed in STMT. The reference
> - is read if IS_READ is true, write otherwise. Returns the
> - data_reference description of MEMREF. NEST is the outermost loop
> - in which the reference should be instantiated, LOOP is the loop in
> - which the data reference should be analyzed. */
> +/* Analyze memory reference MEMREF, which is accessed in STMT.
> + The reference is a read if IS_READ is true, otherwise it is a write.
> + IS_CONDITIONAL_IN_STMT indicates that the reference is conditional
> + within STMT, i.e. that it might not occur even if STMT is executed
> + and runs to completion.
> +
> + Return the data_reference description of MEMREF. NEST is the outermost
> + loop in which the reference should be instantiated, LOOP is the loop
> + in which the data reference should be analyzed. */
>
> struct data_reference *
> create_data_ref (loop_p nest, loop_p loop, tree memref, gimple *stmt,
> - bool is_read)
> + bool is_read, bool is_conditional_in_stmt)
> {
> struct data_reference *dr;
>
> @@ -1148,6 +1152,7 @@ create_data_ref (loop_p nest, loop_p loo
> DR_STMT (dr) = stmt;
> DR_REF (dr) = memref;
> DR_IS_READ (dr) = is_read;
> + DR_IS_CONDITIONAL_IN_STMT (dr) = is_conditional_in_stmt;
>
> dr_analyze_innermost (&DR_INNERMOST (dr), memref,
> nest != NULL ? loop : NULL);
> @@ -4774,6 +4779,11 @@ struct data_ref_loc
>
> /* True if the memory reference is read. */
> bool is_read;
> +
> + /* True if the data reference is conditional within the containing
> + statement, i.e. if it might not occur even when the statement
> + is executed and runs to completion. */
> + bool is_conditional_in_stmt;
> };
>
>
> @@ -4840,6 +4850,7 @@ get_references_in_stmt (gimple *stmt, ve
> {
> ref.ref = op1;
> ref.is_read = true;
> + ref.is_conditional_in_stmt = false;
> references->safe_push (ref);
> }
> }
> @@ -4867,6 +4878,7 @@ get_references_in_stmt (gimple *stmt, ve
> type = TREE_TYPE (gimple_call_arg (stmt, 3));
> if (TYPE_ALIGN (type) != align)
> type = build_aligned_type (type, align);
> + ref.is_conditional_in_stmt = true;
> ref.ref = fold_build2 (MEM_REF, type, gimple_call_arg (stmt, 0),
> ptr);
> references->safe_push (ref);
> @@ -4886,6 +4898,7 @@ get_references_in_stmt (gimple *stmt, ve
> {
> ref.ref = op1;
> ref.is_read = true;
> + ref.is_conditional_in_stmt = false;
> references->safe_push (ref);
> }
> }
> @@ -4899,6 +4912,7 @@ get_references_in_stmt (gimple *stmt, ve
> {
> ref.ref = op0;
> ref.is_read = false;
> + ref.is_conditional_in_stmt = false;
> references->safe_push (ref);
> }
> return clobbers_memory;
> @@ -4963,8 +4977,8 @@ find_data_references_in_stmt (struct loo
>
> FOR_EACH_VEC_ELT (references, i, ref)
> {
> - dr = create_data_ref (nest, loop_containing_stmt (stmt),
> - ref->ref, stmt, ref->is_read);
> + dr = create_data_ref (nest, loop_containing_stmt (stmt), ref->ref,
> + stmt, ref->is_read, ref->is_conditional_in_stmt);
> gcc_assert (dr != NULL);
> datarefs->safe_push (dr);
> }
> @@ -4993,7 +5007,8 @@ graphite_find_data_references_in_stmt (l
>
> FOR_EACH_VEC_ELT (references, i, ref)
> {
> - dr = create_data_ref (nest, loop, ref->ref, stmt, ref->is_read);
> + dr = create_data_ref (nest, loop, ref->ref, stmt, ref->is_read,
> + ref->is_conditional_in_stmt);
> gcc_assert (dr != NULL);
> datarefs->safe_push (dr);
> }
> Index: gcc/tree-ssa-loop-prefetch.c
> ===================================================================
> --- gcc/tree-ssa-loop-prefetch.c 2017-07-27 13:46:58.376642483 +0100
> +++ gcc/tree-ssa-loop-prefetch.c 2017-07-27 13:46:58.578633377 +0100
> @@ -1633,7 +1633,7 @@ determine_loop_nest_reuse (struct loop *
> for (ref = gr->refs; ref; ref = ref->next)
> {
> dr = create_data_ref (nest, loop_containing_stmt (ref->stmt),
> - ref->mem, ref->stmt, !ref->write_p);
> + ref->mem, ref->stmt, !ref->write_p, false);
>
> if (dr)
> {
> Index: gcc/tree-vect-data-refs.c
> ===================================================================
> --- gcc/tree-vect-data-refs.c 2017-07-27 13:46:58.376642483 +0100
> +++ gcc/tree-vect-data-refs.c 2017-07-27 13:46:58.579633332 +0100
> @@ -708,6 +708,69 @@ vect_slp_analyze_instance_dependence (sl
> return res;
> }
>
> +/* Record in VINFO the base alignment guarantee given by DRB. STMT is
> + the statement that contains DRB, which is useful for recording in the
> + dump file. */
> +
> +static void
> +vect_record_base_alignment (vec_info *vinfo, gimple *stmt,
> + innermost_loop_behavior *drb)
> +{
> + bool existed;
> + innermost_loop_behavior *&entry
> + = vinfo->base_alignments.get_or_insert (drb->base_address, &existed);
> + if (!existed || entry->base_alignment < drb->base_alignment)
> + {
> + entry = drb;
> + if (dump_enabled_p ())
> + {
> + dump_printf_loc (MSG_NOTE, vect_location,
> + "recording new base alignment for ");
> + dump_generic_expr (MSG_NOTE, TDF_SLIM, drb->base_address);
> + dump_printf (MSG_NOTE, "\n");
> + dump_printf_loc (MSG_NOTE, vect_location,
> + " alignment: %d\n", drb->base_alignment);
> + dump_printf_loc (MSG_NOTE, vect_location,
> + " misalignment: %d\n", drb->base_misalignment);
> + dump_printf_loc (MSG_NOTE, vect_location,
> + " based on: ");
> + dump_gimple_stmt (MSG_NOTE, TDF_SLIM, stmt, 0);
> + }
> + }
> +}
> +
> +/* If the region we're going to vectorize is reached, all unconditional
> + data references occur at least once. We can therefore pool the base
> + alignment guarantees from each unconditional reference. Do this by
> + going through all the data references in VINFO and checking whether
> + the containing statement makes the reference unconditionally. If so,
> + record the alignment of the base address in VINFO so that it can be
> + used for all other references with the same base. */
> +
> +void
> +vect_record_base_alignments (vec_info *vinfo)
> +{
> + loop_vec_info loop_vinfo = dyn_cast <loop_vec_info> (vinfo);
> + struct loop *loop = loop_vinfo ? LOOP_VINFO_LOOP (loop_vinfo) : NULL;
> + data_reference *dr;
> + unsigned int i;
> + FOR_EACH_VEC_ELT (vinfo->datarefs, i, dr)
> + if (!DR_IS_CONDITIONAL_IN_STMT (dr))
> + {
> + gimple *stmt = DR_STMT (dr);
> + vect_record_base_alignment (vinfo, stmt, &DR_INNERMOST (dr));
> +
> + /* If DR is nested in the loop that is being vectorized, we can also
> + record the alignment of the base wrt the outer loop. */
> + if (loop && nested_in_vect_loop_p (loop, stmt))
> + {
> + stmt_vec_info stmt_info = vinfo_for_stmt (stmt);
> + vect_record_base_alignment
> + (vinfo, stmt, &STMT_VINFO_DR_WRT_VEC_LOOP (stmt_info));
> + }
> + }
> +}
> +
> /* Function vect_compute_data_ref_alignment
>
> Compute the misalignment of the data reference DR.
> @@ -725,6 +788,7 @@ vect_compute_data_ref_alignment (struct
> {
> gimple *stmt = DR_STMT (dr);
> stmt_vec_info stmt_info = vinfo_for_stmt (stmt);
> + vec_base_alignments *base_alignments = &stmt_info->vinfo->base_alignments;
> loop_vec_info loop_vinfo = STMT_VINFO_LOOP_VINFO (stmt_info);
> struct loop *loop = NULL;
> tree ref = DR_REF (dr);
> @@ -793,6 +857,15 @@ vect_compute_data_ref_alignment (struct
> unsigned int base_misalignment = drb->base_misalignment;
> unsigned HOST_WIDE_INT vector_alignment = TYPE_ALIGN_UNIT (vectype);
>
> + /* Calculate the maximum of the pooled base address alignment and the
> + alignment that we can compute for DR itself. */
> + innermost_loop_behavior **entry = base_alignments->get (drb->base_address);
> + if (entry && base_alignment < (*entry)->base_alignment)
> + {
> + base_alignment = (*entry)->base_alignment;
> + base_misalignment = (*entry)->base_misalignment;
> + }
> +
> if (drb->offset_alignment < vector_alignment
> || !step_preserves_misalignment_p
> /* We need to know whether the step wrt the vectorized loop is
> @@ -2113,8 +2186,7 @@ vect_find_same_alignment_drs (struct dat
> if (dra == drb)
> return;
>
> - if (!operand_equal_p (DR_BASE_OBJECT (dra), DR_BASE_OBJECT (drb),
> - OEP_ADDRESS_OF)
> + if (!operand_equal_p (DR_BASE_ADDRESS (dra), DR_BASE_ADDRESS (drb), 0)
> || !operand_equal_p (DR_OFFSET (dra), DR_OFFSET (drb), 0)
> || !operand_equal_p (DR_STEP (dra), DR_STEP (drb), 0))
> return;
> @@ -2172,6 +2244,7 @@ vect_analyze_data_refs_alignment (loop_v
> vec<data_reference_p> datarefs = vinfo->datarefs;
> struct data_reference *dr;
>
> + vect_record_base_alignments (vinfo);
> FOR_EACH_VEC_ELT (datarefs, i, dr)
> {
> stmt_vec_info stmt_info = vinfo_for_stmt (DR_STMT (dr));
> @@ -3437,7 +3510,8 @@ vect_analyze_data_refs (vec_info *vinfo,
> {
> struct data_reference *newdr
> = create_data_ref (NULL, loop_containing_stmt (stmt),
> - DR_REF (dr), stmt, maybe_scatter ? false : true);
> + DR_REF (dr), stmt, !maybe_scatter,
> + DR_IS_CONDITIONAL_IN_STMT (dr));
> gcc_assert (newdr != NULL && DR_REF (newdr));
> if (DR_BASE_ADDRESS (newdr)
> && DR_OFFSET (newdr)
> Index: gcc/tree-vect-slp.c
> ===================================================================
> --- gcc/tree-vect-slp.c 2017-07-27 13:46:58.376642483 +0100
> +++ gcc/tree-vect-slp.c 2017-07-27 13:46:58.579633332 +0100
> @@ -2764,6 +2764,8 @@ vect_slp_analyze_bb_1 (gimple_stmt_itera
> return NULL;
> }
>
> + vect_record_base_alignments (bb_vinfo);
> +
> /* Analyze and verify the alignment of data references and the
> dependence in the SLP instances. */
> for (i = 0; BB_VINFO_SLP_INSTANCES (bb_vinfo).iterate (i, &instance); )
> Index: gcc/testsuite/gcc.dg/vect/pr81136.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/vect/pr81136.c 2017-07-27 13:46:58.376642483 +0100
> +++ gcc/testsuite/gcc.dg/vect/pr81136.c 2017-07-27 13:46:58.577633422 +0100
> @@ -14,3 +14,5 @@ fn1 (int n)
> for (int i = 0; i < n; i++)
> a->bar[i] = b[i];
> }
> +
> +/* { dg-final { scan-tree-dump-not "Unknown misalignment" "vect" } } */