On Mon, Jul 25, 2011 at 4:03 PM, Richard Guenther
<richard.guent...@gmail.com> wrote:
> On Mon, Jul 25, 2011 at 3:24 PM, Richard Guenther
> <richard.guent...@gmail.com> wrote:
>> On Mon, Jul 25, 2011 at 3:22 PM, Ulrich Weigand <uweig...@de.ibm.com> wrote:
>>> Richard Guenther wrote:
>>>
>>>> >>>>>> Well, the back-end assumes a pointer to vector type is always
>>>> >>>>>> naturally aligned, and therefore the data it points to can be
>>>> >>>>>> accessed via a simple load, with no extra rotate needed.
>>>> >>>>>
>>>> >>>>> I can't see any use of VECTOR_TYPE in config/spu/, and assuming
>>>> >>>>> anything about alignment just because of the kind of the pointer
>>>> >>>>> is bogus - the scalar code does a scalar read using that pointer.
>>>> >>>>> So the backend better should look at the memory operation, not
>>>> >>>>> at the pointer type.  That said, I can't find any code that looks
>>>> >>>>> suspicious in the spu backend.
>>>> >>>>>
>>>> >>>>>> It seems what happened here is that somehow, a pointer to int
>>>> >>>>>> gets replaced by a pointer to vector, even though their alignment
>>>> >>>>>> properties are different.
>>>> >>>>>
>>>> >>>>> No, they are not.  They get replaced if they are value-equivalent
>>>> >>>>> in which case they are also alignment-equivalent.  But well, the
>>>> >>>>> dump snippet wasn't complete and I don't feel like building a
>>>> >>>>> SPU cross to verify myself.
>>>
>>>> > Seems perfectly valid to me.  Or well - I suppose we might run into
>>>> > the issue that the vectorizer sets alignment data at the wrong spot?
>>>> > You can check alignment info when dumping with the -alias flag.
>>>> > Building a spu cross now.
>>>>
>>>> Nope, all perfectly valid.
>>>
>>> Ah, I guess I see what's happening here.  When the SPU back-end is called
>>> to expand the load, the source operand is passed as:
>>>
>>> (mem:SI (reg/f:SI 226 [ vect_pa.9 ])
>>>        [2 MEM[base: vect_pa.9_44, offset: 0B]+0 S4 A32])
>>>
>>> Now this does say the MEM is only guaranteed to be aligned to 32 bits.
>>>
>>> However, spu_expand_load then goes and looks at the components of the
>>> address in detail, in order to figure out how to best perform the access.
>>> In doing so, it looks at the REGNO_POINTER_ALIGN values of the base
>>> registers involved in the address.
>>>
>>> In this case, REGNO_POINTER_ALIGN (226) is set to 128, and therefore
>>> the back-end thinks it can use an aligned access after all.
>>>
>>> Now, the reason why REGNO_POINTER_ALIGN (226) is 128 is that the register
>>> is the DECL_RTL for the variable vect_pa.9, and that variable has a
>>> pointer-to-vector type (with target alignment 128).
>>>
>>> When expanding that variable, expand_one_register_var does:
>>>
>>>  if (POINTER_TYPE_P (type))
>>>    mark_reg_pointer (x, TYPE_ALIGN (TREE_TYPE (type)));
>>>
>>> All this is normally completely correct -- a variable of type pointer
>>> to vector type *must* hold only properly aligned values.
>>
>> No, this is indeed completely bogus code ;)  it should instead
>> use get_pointer_alignment.
>
> Btw, as pseudos do not have a single def site how can the above
> ever be correct in the face of coalescing?  For example on trees we
> can have
>
>  p_1 = &a; // align 256
>  p_2 = p_1 + 4; // align 32
>
> but we'll coalesce the thing and thus would have to use the weaker
> alignment of both SSA names.  expand_one_register_var expands
> the decl, not the SSA name, so using get_pointer_alignment on
> the decl would probably be fine, though also pointless as it always
> will return 8.
>
> At least I don't see any code that would prevent a temporary variable
> of type int * of being coalesced with a temporary variable of type vector int 
> *.
>
> Why should REGNO_POINTER_ALIGN be interesting to anyone?
> Proper alignment information is (should be) attached to every
> MEM already.

nonzero_bits1 seems to be the only consumer of REGNO_POINTER_ALIGN
apart from maybe alpha.c and spu.c.

We should simply kill REGNO_POINTER_ALIGN IMHO.

Richard.

> Richard.
>

Reply via email to