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. Richard.
