This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [patch] Fix virtual operands creation in vectorizer
Hello,
> > Maybe the code in tree-data-ref.c that computes the memtag should be
> > updated - I don't think it was updated since incorporation of
> structalias,
> > so it might record the wrong tag (the tag for the whole struct instead of
> > the tag for the specific field).
> >
>
> Does this patch make sense?
>
> The patch introduces one change to the function that creates new type-tags
> for the newly created vector pointers: instead of adding all the subvars of
> 'var' as may-aliases of the new tag, we add only the subvar whose offset is
> equal to 'field_bit_offset' (a new argument to the function).
>
> For now I have a gcc_assert to make sure that 'field_bit_offset' matches
> the offset of at least one subvar. We may want to change this to fall back
> to the original behavior (add all the subvars as may-aliases) if no
> matching subvar was found. For now the assert is there because it catches a
> bug in a couple of vectorizer testcases (for some reason the field
> DR_OFFSET holds what seems to be a wrong value. I haven't debugged that
> yet).
>
> Looking at the dumps, with this patch only the relevant virtual defs are
> created for the accesses through these vector-pointers with fixed alias
> information.
>
> Yet to be tested and bootstrapped, but for now - does this approach look
> ok?
you definitely should add update_stmt calls to all changed statements
(basically on all the places where my patch has
mark_new_vars_to_rename). That way, you should also notice any problems
more easily (f you fail to set up right tags, it should get caught
in ssa form verification).
Zdenek
> * tree-vect-transform.c (vect_create_data_ref_ptr): Call new_type_alias
> with additional argument.
> * tree-flow.h (new_type_alias): Takes additional argument.
> * tree-ssa-alias.c (new_type_alias): New argument. Add only the relevant
> subvar when possible.
>
> (See attached file: vecptr-alias.patch.txt)
>
> > dorit
> >
> > > Instead, the patch below implements a
> > > conservative solution that marks the new virtual operands of these
> > > statements for renaming. As this loses some information, which may
> > > prevent further optimizations, someone more familiar with the
> vectorizer
> > > should look to it and implement more precise solution later.
> > >
> > > Fixing this bug also makes it unnecessary to have
> > > mark_new_vars_to_rename in tree-ssa-loop-ivopts.c:rewrite_use. As the
> > > idea to add this call there was discussed several times, and this time
> > the
> > > call was introduced there purely to mask this bug in vectorizer, I have
> > also
> > > added a short comment explaining why adding it is a bad idea.
> > >
> > > Bootstrapped & regtested on i686, x86_64 and ia64.
> > >
> > > Zdenek
> > >
> > > * tree-ssa-loop-ivopts.c (rewrite_use): Call update_stmt instead of
> > > mark_new_vars_to_rename. Explain why.
> > > * tree-ssa-loop.c (pass_iv_optimize): Remove TODO_update_ssa.
> > > * tree-vect-transform.c (vectorizable_store, vectorizable_load,
> > > vectorizable_load): Add mark_new_vars_to_rename calls for the
> modified
> > > statements.
> > >
> > > Index: tree-ssa-loop-ivopts.c
> > > ===================================================================
> > > *** tree-ssa-loop-ivopts.c (revision 109451)
> > > --- tree-ssa-loop-ivopts.c (working copy)
> > > *************** rewrite_use (struct ivopts_data *data,
> > > *** 5822,5828 ****
> > > default:
> > > gcc_unreachable ();
> > > }
> > > ! mark_new_vars_to_rename (use->stmt);
> > > }
> > >
> > > /* Rewrite the uses using the selected induction variables. */
> > > --- 5822,5861 ----
> > > default:
> > > gcc_unreachable ();
> > > }
> > > !
> > > ! /* rewrite_use_address rewrites memory references to
> TARGET_MEM_REFs.
> > > ! Ensuring that the TARGET_MEM_REF gets the same set of virtual
> > > operands as
> > > ! the original memory reference is complicated (mainly because
> > > the process
> > > ! of determining the virtual operands for the original memory
> > > reference is
> > > ! nontrivial, see tree-ssa-operands.c:get_expr_operands), and
> > > from time to
> > > ! time (especially when something related to alias analysis
> > changes), the
> > > ! so far used approach may fail and get_ref_tag needs to be
> updated
> > to
> > > ! handle the new situation. One might ask ``why bother, just put
> > > ! mark_new_vars_to_rename (use->stmt) here and TODO_update_ssa
> > > ! to the flags of this pass, and you are done.'' This is not a
> good
> > idea,
> > > ! for the following reasons:
> > > !
> > > ! 1) Effectivity -- while the update_ssa call usually is not
> > > very expensive,
> > > ! calling it without a good reason still is not good.
> > > ! 2) Marking vars for renaming here does something in two cases:
> > > ! a) the new virtual operands of the reference are a strict superset
> > of
> > > ! the old ones. This means that we have lost some information
> (we
> > now
> > > ! believe this memory reference to alias more things than
> before),
> > and
> > > ! this may prevent other optimizations.
> > > ! b) the new virtual operands are not a superset of the old ones.
> > This
> > > ! is a real problem, as we now wrongly believe the ref not to
> alias
> > > ! some of the references, which may lead to hard to debug
> > > ! misscompilations. Getting an ICE is much better in this case.
> > > ! 3) In many cases, the problem is not in ivopts, but in some
> > > earlier pass
> > > ! that failed to update virtual operands correctly (the reason
> usually
> > is
> > > ! a forgotten call to update_stmt or mark_new_vars_to_rename).
> > > ! Masking such a bug with mark_new_vars_to_rename call here only
> > > ! causes it to show up again later.
> > > !
> > > ! So if you run into problems that cause you to believe that you
> > need to
> > > ! have mark_new_vars_to_rename here, please investigate and fix
> the
> > real
> > > ! cause of the problem instead. */
> > > ! update_stmt (use->stmt);
> > > }
> > >
> > > /* Rewrite the uses using the selected induction variables. */
> > > Index: tree-ssa-loop.c
> > > ===================================================================
> > > *** tree-ssa-loop.c (revision 109451)
> > > --- tree-ssa-loop.c (working copy)
> > > *************** struct tree_opt_pass pass_iv_optimize =
> > > *** 433,440 ****
> > > 0, /* properties_destroyed */
> > > 0, /* todo_flags_start */
> > > TODO_dump_func
> > > ! | TODO_verify_loops
> > > ! | TODO_update_ssa, /* todo_flags_finish */
> > > 0 /* letter */
> > > };
> > >
> > > --- 433,439 ----
> > > 0, /* properties_destroyed */
> > > 0, /* todo_flags_start */
> > > TODO_dump_func
> > > ! | TODO_verify_loops, /* todo_flags_finish */
> > > 0 /* letter */
> > > };
> > >
> > > Index: tree-vect-transform.c
> > > ===================================================================
> > > *** tree-vect-transform.c (revision 109451)
> > > --- tree-vect-transform.c (working copy)
> > > *************** vectorizable_store (tree stmt, block_stm
> > > *** 1592,1597 ****
> > > --- 1592,1598 ----
> > > renaming so the later use will also be renamed. */
> > > mark_sym_for_renaming (SSA_NAME_VAR (def));
> > > }
> > > + mark_new_vars_to_rename (*vec_stmt);
> > >
> > > return true;
> > > }
> > > *************** vectorizable_load (tree stmt, block_stmt
> > > *** 1707,1712 ****
> > > --- 1708,1714 ----
> > > TREE_OPERAND (new_stmt, 0) = new_temp;
> > > vect_finish_stmt_generation (stmt, new_stmt, bsi);
> > > copy_virtual_operands (new_stmt, stmt);
> > > + mark_new_vars_to_rename (new_stmt);
> > > }
> > > else if (alignment_support_cheme == dr_unaligned_software_pipeline)
> > > {
> > > *************** vectorizable_load (tree stmt, block_stmt
> > > *** 1746,1752 ****
> > > msq_init = TREE_OPERAND (new_stmt, 0);
> > > copy_virtual_operands (new_stmt, stmt);
> > > update_vuses_to_preheader (new_stmt, loop);
> > > !
> > >
> > > /* <2> Create lsq = *(floor(p2')) in the loop */
> > > offset = size_int (TYPE_VECTOR_SUBPARTS (vectype) - 1);
> > > --- 1748,1754 ----
> > > msq_init = TREE_OPERAND (new_stmt, 0);
> > > copy_virtual_operands (new_stmt, stmt);
> > > update_vuses_to_preheader (new_stmt, loop);
> > > ! mark_new_vars_to_rename (new_stmt);
> > >
> > > /* <2> Create lsq = *(floor(p2')) in the loop */
> > > offset = size_int (TYPE_VECTOR_SUBPARTS (vectype) - 1);
> > > *************** vectorizable_load (tree stmt, block_stmt
> > > *** 1759,1765 ****
> > > vect_finish_stmt_generation (stmt, new_stmt, bsi);
> > > lsq = TREE_OPERAND (new_stmt, 0);
> > > copy_virtual_operands (new_stmt, stmt);
> > > !
> > >
> > > /* <3> */
> > > if (targetm.vectorize.builtin_mask_for_load)
> > > --- 1761,1767 ----
> > > vect_finish_stmt_generation (stmt, new_stmt, bsi);
> > > lsq = TREE_OPERAND (new_stmt, 0);
> > > copy_virtual_operands (new_stmt, stmt);
> > > ! mark_new_vars_to_rename (new_stmt);
> > >
> > > /* <3> */
> > > if (targetm.vectorize.builtin_mask_for_load)
> > >
> >
> >
> Index: tree-vect-transform.c
> ===================================================================
> --- tree-vect-transform.c (revision 109472)
> +++ tree-vect-transform.c (working copy)
> @@ -302,12 +303,26 @@
> /* If tag is a variable (and NOT_A_TAG) than a new type alias
> tag must be created with tag added to its may alias list. */
> if (!MTAG_P (tag))
> - new_type_alias (vect_ptr, tag);
> + {
> + tree field_bit_offset = NULL_TREE;
> + tree base_obj = DR_BASE_OBJECT (dr);
> + tree base_addr = DR_BASE_ADDRESS (dr);
> +
> + if (base_obj && TREE_CODE (base_obj) == COMPONENT_REF
> + && TREE_CODE (TREE_OPERAND (base_obj, 1)) == FIELD_DECL
> + && TREE_CODE (base_addr) == ADDR_EXPR
> + && TREE_OPERAND (base_addr, 0) == tag)
> + field_bit_offset = size_binop (MULT_EXPR, DR_OFFSET (dr),
> + ssize_int (BITS_PER_UNIT));
> +
> + new_type_alias (vect_ptr, tag, field_bit_offset);
> + }
> else
> var_ann (vect_ptr)->type_mem_tag = tag;
>
> var_ann (vect_ptr)->subvars = DR_SUBVARS (dr);
>
> +
> /** (3) Calculate the initial address the vector-pointer, and set
> the vector-pointer to point to it before the loop: **/
>
> Index: tree-flow.h
> ===================================================================
> --- tree-flow.h (revision 109472)
> +++ tree-flow.h (working copy)
> @@ -588,7 +588,7 @@
> extern bool is_aliased_with (tree, tree);
> extern struct ptr_info_def *get_ptr_info (tree);
> extern void add_type_alias (tree, tree);
> -extern void new_type_alias (tree, tree);
> +extern void new_type_alias (tree, tree, tree);
> extern void count_uses_and_derefs (tree, tree, unsigned *, unsigned *, bool *);
> static inline subvar_t get_subvars_for_var (tree);
> static inline tree get_subvar_at (tree, unsigned HOST_WIDE_INT);
> Index: tree-ssa-alias.c
> ===================================================================
> --- tree-ssa-alias.c (revision 109590)
> +++ tree-ssa-alias.c (working copy)
> @@ -2343,13 +2343,16 @@
>
>
> /* Create a new type tag for PTR. Construct the may-alias list of this type
> - tag so that it has the aliasing of VAR.
> + tag so that it has the aliasing of VAR. If field_bit_offset is also
> + passed, it is used to set the aliasing information more accurately by
> + indicating the bit-offset of the only subvar that should be added as
> + may_alias of the tag.
>
> Note, the set of aliases represented by the new type tag are not marked
> for renaming. */
>
> void
> -new_type_alias (tree ptr, tree var)
> +new_type_alias (tree ptr, tree var, tree field_bit_offset)
> {
> var_ann_t p_ann = var_ann (ptr);
> tree tag_type = TREE_TYPE (TREE_TYPE (ptr));
> @@ -2370,8 +2373,27 @@
> tag = create_memory_tag (tag_type, true);
> p_ann->type_mem_tag = tag;
>
> - for (sv = svars; sv; sv = sv->next)
> - add_may_alias (tag, sv->var);
> + if (field_bit_offset && (TREE_CODE (field_bit_offset) == INTEGER_CST))
> + {
> + bool found_it = false;
> +
> + for (sv = svars; sv; sv = sv->next)
> + {
> + if (tree_int_cst_compare (bitsize_int (sv->offset),
> + field_bit_offset) == 0)
> + {
> + add_may_alias (tag, sv->var);
> + found_it = true;
> + break;
> + }
> + }
> + gcc_assert (found_it);
> + }
> + else
> + {
> + for (sv = svars; sv; sv = sv->next)
> + add_may_alias (tag, sv->var);
> + }
> }
> else
> {