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