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

[Richard Henderson <rth at redhat dot com>]

On Thu, Dec 23, 2004 at 10:01:37AM +0200, Ira Rosen wrote:
> > >       /* 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))).

That's not all of it, obviously.  The SSA_NAME is being tested
for pointer-ness, while VAR_DECL isn't.  Now, I understand that
you're trying to handle both static data access and heap access,
and that the former will eventually consist of VAR_DECLs with
VOPs and such, and that the later will eventually reference 
pointers which do get SSA renamed.

HOWEVER, I'm confused about the mix of data types here.  On the
one hand you've got an object (the VAR_DECL), and on the other
hand you've got a pointer to an object (the SSA_NAME).  I would
think that you'd actually be examining VAR_DECLs and INDIRECT_REFs,
since both of these are objects.

However, I think this is a pre-existing bit of confusion that
should not be addressed with this patch.

> 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?

Yes, this is good enough for the debugging assert.

>       /* 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)
> ! 	  && TYPE_ALIGN (TREE_TYPE (expr)) >= TYPE_ALIGN (vectype))
>   	*base_aligned_p = true;

What you had before was correct; this one isn't.  You don't need
to check TYPE_ALIGN (TREE_TYPE (expr)).  DECL_ALIGN is guaranteed
to be the correct value for this decl.  In particular, DECL_ALIGN
can be greater than TYPE_ALIGN due to __attribute__((align(N))).

> !       *initial_offset = convert (sizetype, expr);
> !       *misalign = convert (sizetype, expr);

Again, use fold_convert.

> !       bit_pos_in_bytes = build_int_cst (sizetype, pbitpos/BITS_PER_UNIT); 

This is the same as size_int(pbitpos/BITS_PER_UNIT).  There are lots
of other ocurrences.

> --- 1875,1891 ----
>     is_addr_expr = TREE_CODE (data_ref_base) == ADDR_EXPR
>                    || TREE_CODE (data_ref_base) == PLUS_EXPR
>                    || TREE_CODE (data_ref_base) == MINUS_EXPR;
> !   is_comp_ref = TREE_CODE (data_ref_base_type) == RECORD_TYPE; 
>   
> !   gcc_assert (is_ptr_ref || is_array_ref || is_addr_expr || is_comp_ref);
>   
> !   if (is_array_ref || is_comp_ref)
> !     /* Add '&' to ref_base.  */
> !     data_ref_base = build_fold_addr_expr (data_ref_base);

I'm thinking maybe you *did* mean pointer type before.  Because now
you're not handling complex numbers.  Change it back and add a 
comment explaining the alternatives.

Again I find this mix of object and pointer to be confusing, and 
again something to be cleaned up by a different patch.


r~