This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] Fix PR/18179 and use get_inner_reference in vectorizer: part 4






> > +   tree unit_bits = fold_convert (unsigned_type_node,
> > +              build_int_cst (NULL_TREE, BITS_PER_UNIT));
>
> This is
>
>    unit_bits = build_int_cst (unsigned_type_node, BITS_PER_UNIT);
>
> HOWEVER, usage of unsigned_type_node is highly suspect, as it's
> normally a 32-bit type, and what are you doing with 32-bit types
> on what could be a 64-bit system?
>
> My guess is that you actually want bitsize_int(BITS_PER_UNIT), and
> most of your int_const_binop invocations should be using size_binop.
>

unit_bits is redundant now, since I did bit offset arithmetic in plain C
as you recommended.

> Note the existance of both bitsizetype and sizetype.  The former,
> obviously, should be used when referring to bit offsets, and the
> later when referring to byte offsets.  For reference, these will
> correspond to the types of TYPE_SIZE and TYPE_SIZE_UNIT respectively.
>
> >       /* These cases continue the recursion:  */
> >       case COMPONENT_REF:
> > !     case ARRAY_REF:
>
> You should use handled_component_p to decide what to send to
> get_inner_reference.  You're not handling REALPART_EXPR, among
> others.  Using handled_component_p ensures that you stay in sync.

Done.

>
> >       /* These cases end the recursion:  */
> >       case VAR_DECL:
> >       case PARM_DECL:
> > !       *initial_offset = size_zero_node;
> > !       *step = size_zero_node;
> > !       *misalign = size_zero_node;
> > !       if (DECL_ALIGN (expr) >= TYPE_ALIGN (vectype))
> >      *base_aligned_p = true;
> >         return expr;
> >
> >       case SSA_NAME:
> >         if (TREE_CODE (TREE_TYPE (expr)) != POINTER_TYPE)
> > !      return NULL_TREE;
>
> I don't really understand why VAR_DECL and SSA_NAME are being
> treated differently.

The problem is that to get alignment for VAR_DECL I need
TYPE_ALIGN (TREE_TYPE (expr)) and for SSA_NAME TYPE_ALIGN (TREE_TYPE
(TREE_TYPE (expr))).


>
> > !       align = fold_convert (unsigned_type_node,
> > !              build_int_cst (NULL_TREE,
> > !                   GET_MODE_SIZE (TYPE_MODE (vectype))));
>
> See above.  Of course, this begs the question of what
>
> > !      && !vect_analyze_offset_expr (poffset, loop, align,
&this_offset,
> > !                &this_misalign, &this_step))
>
> this function is doing with its alignment parameter, and whether
> there are further corrections to be made.

Fixed.

>
> > !       bit_pos = build_int_cst (NULL_TREE, pbitpos);
> > !       bit_pos_in_bytes = int_const_binop (TRUNC_DIV_EXPR, bit_pos,
> > !                  unit_bits, 1);
> > !       /* Check that there is no remainder in bits.  */
> > !       bit_offset = int_const_binop (TRUNC_MOD_EXPR, bit_pos,
> unit_bits, 1);
> > !       if (!integer_zerop (bit_offset))
>
> Note that you already know that pbitpos is a HOST_WIDE_INT.  You
> can do all this arithmetic in plain C.

OK.

>
> > !       this_offset = fold (build2 (PLUS_EXPR, TREE_TYPE (this_offset),

> > !               this_offset,
> > !               bit_pos_in_bytes));
>
> This should definitely be using size_binop.

OK.

>
> > !       if (!host_integerp (*step, 1) || TREE_OVERFLOW (*step))
>
> More incorrect uses of host_integerp.  You should also know that pointer
> arithmetic is assumed to never overflow.

OK.

>
> > !   if (TREE_CODE (TREE_TYPE (data_ref_base)) != POINTER_TYPE)
> > !     /* Add '&' to ref_base, if it is not a pointer.  */
> > !     data_ref_base = build_fold_addr_expr (data_ref_base);
>
> Perhaps you mean
>
>    TREE_CODE (TREE_TYPE (data_ref_base)) == ARRAY_TYPE
>
> ?  I can't see what you're doing being valid for just any random
non-pointer.

Fixed. We do this for COMPONENT_REF too.


>
> > !   tree type_size = build_int_cst (NULL_TREE, GET_MODE_SIZE
> (TYPE_MODE (scalar_type)));
> >
> > !   if (!step || (!simple_cst_equal (type_size, step) && !
> integer_zerop (step)))
>
>   tree_int_cst_compare (step, TYPE_SIZE_UNIT (scalar_type));

OK.

>
> Please examine every place referencing GET_MODE_SIZE.

I replaced GET_MODE_SIZE with TYPE_SIZE_UNIT except the places where we
need a number and not
a tree. Like here:

gcc_assert (GET_MODE_SIZE (TYPE_MODE (scalar_type))
                  * vectorization_factor == UNITS_PER_SIMD_WORD);
Is this OK?

>
> > +   STMT_VINFO_VECT_STEP (stmt_info) = build_int_cst (NULL_TREE,
step_val);
>
> Please examine every place using build_int_cst with a NULL first
> argument.  All of them are wrong.
>

Done.

> > +   if (TREE_CODE (init) == PLUS_EXPR
> > +       || TREE_CODE (init) == MINUS_EXPR)
> > +     STMT_VINFO_VECT_INIT_OFFSET (stmt_info) =
> > +       fold (build2 (TREE_CODE (init), TREE_TYPE (TREE_OPERAND (init,
1)),
> > +           size_zero_node, TREE_OPERAND (init, 1)));
> > +   else
> > +     STMT_VINFO_VECT_INIT_OFFSET (stmt_info) = size_zero_node;
>
> Note here that STMT_VINFO_VECT_INIT_OFFSET winds up with different
> types depending on which path taken.  This is wrong.
>

OK.
>
>
> r~

Thanks again for your comments.


Bootstrapped and tested (including Spec) on ppc-darwin.
OK for mainline?

Ira

Patch 4 (new):
(See attached file: patch4New)

Diff between new and old patch 4:
(See attached file: patch4.diff)

Attachment: patch4New
Description: Binary data

Attachment: patch4.diff
Description: Binary data


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]