| Message ID | 3258931.AVZnoAGDHX@polaris |
|---|---|
| State | New |
| Headers | show |
On Sat, Jan 11, 2014 at 12:42 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: > [Sorry for dropping the ball here] > >> I think that may_be_unaligned_p is just seriously out-dated ... shouldn't it >> be sth like >> >> get_object_alignment_1 (ref, &align, &bitpos); >> if step * BITS_PER_UNIT + bitpos is misaligned >> ... >> >> or rather all this may_be_unaligned_p stuff should be dropped and IVOPTs >> should finally generate proper [TARGET_]MEM_REFs instead? That is, >> we already handle aliasing fine: >> >> ref = create_mem_ref (&bsi, TREE_TYPE (*use->op_p), &aff, >> reference_alias_ptr_type (*use->op_p), >> iv, base_hint, data->speed); >> >> so just also handle alignment properly by passing down >> get_object_alignment (*use->op_p) and in create_mem_ref_raw >> do at the end do the >> >> if (TYPE_MODE (type) != BLKmode >> && GET_MODE_ALIGNMENT (TYPE_MODE (type)) > align) >> type = build_aligned_type (type, align); >> >> for BLKmode we already look at TYPE_ALIGN and as we do not change >> the access type(?) either the previous code was already wrong or it was >> fine, so there is nothing to do. >> >> So - if you want to give it a try...? > > After a bit of pondering, I'm not really thrilled, as this would mean changing > TARGET_MEM_REF to accept invalid (unaligned) memory references for the target. AFAIK the expander already handles this if the target can expand it via movmisalign at least. One issue with vectorization is that possibly unaligned vector accesses are not handled/optimized by IVOPTs which is bad. Something to re-visit for 4.10. > But I agree that may_be_unaligned_p is seriously outdated, so the attached > patch entirely rewrites it, fixing the bug in the process. > > Tested on SPARC, SPARC64, IA-64 and ARM, OK for the mainline? OK. Note that this now lets unaligned vector moves slip through as their TYPE_ALIGN (TREE_TYPE (ref)) is properly reflecting this fact, so is anything which dereferences a type with an aligned attribute lowering its alignment. Which of course raises the question what the function is supposed to verify alignment against - given that it is only queried for STRICT_ALIGNMENT targets I would guess it wants to verify against mode alignment (historically at least ...). Not sure how this observation relates to the bug you want to fix though. Still the patch is an improvement and thus ok. Thanks, Richard. > 2014-01-10 Eric Botcazou <ebotcazou@adacore.com> > > * builtins.c (get_object_alignment_2): Minor tweak. > * tree-ssa-loop-ivopts.c (may_be_unaligned_p): Rewrite. > > > -- > Eric Botcazou
> Note that this now lets unaligned vector moves slip through as > their TYPE_ALIGN (TREE_TYPE (ref)) is properly reflecting this > fact, so is anything which dereferences a type with an aligned > attribute lowering its alignment. > > Which of course raises the question what the function is > supposed to verify alignment against - given that it is only > queried for STRICT_ALIGNMENT targets I would guess > it wants to verify against mode alignment (historically > at least ...). Not sure how this observation relates to the > bug you want to fix though. Yes, it was the mode, but on STRICT_ALIGNMENT targets types must be as aligned as their mode (unless you previously under-aligned the type and knew what you were doing when you did it...). The bug is that, for BLKmode, you really need to look at the type to have the alignment. > Still the patch is an improvement and thus ok. Thanks.
On Mon, Jan 13, 2014 at 11:37 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> Note that this now lets unaligned vector moves slip through as >> their TYPE_ALIGN (TREE_TYPE (ref)) is properly reflecting this >> fact, so is anything which dereferences a type with an aligned >> attribute lowering its alignment. >> >> Which of course raises the question what the function is >> supposed to verify alignment against - given that it is only >> queried for STRICT_ALIGNMENT targets I would guess >> it wants to verify against mode alignment (historically >> at least ...). Not sure how this observation relates to the >> bug you want to fix though. > > Yes, it was the mode, but on STRICT_ALIGNMENT targets types must be as aligned > as their mode (unless you previously under-aligned the type and knew what you > were doing when you did it...). Yeah, the vectorizer first querying target capabilities and then under-aligning the vector type probably qualifies here. > The bug is that, for BLKmode, you really need > to look at the type to have the alignment. Of course. >> Still the patch is an improvement and thus ok. > > Thanks. > > -- > Eric Botcazou
Index: builtins.c =================================================================== --- builtins.c (revision 206455) +++ builtins.c (working copy) @@ -434,7 +434,7 @@ get_object_alignment_2 (tree exp, unsign alignment that can prevail. */ if (offset) { - int trailing_zeros = tree_ctz (offset); + unsigned int trailing_zeros = tree_ctz (offset); if (trailing_zeros < HOST_BITS_PER_INT) { unsigned int inner = (1U << trailing_zeros) * BITS_PER_UNIT; Index: tree-ssa-loop-ivopts.c =================================================================== --- tree-ssa-loop-ivopts.c (revision 206455) +++ tree-ssa-loop-ivopts.c (working copy) @@ -1668,50 +1668,30 @@ constant_multiple_of (tree top, tree bot } } -/* Returns true if memory reference REF with step STEP may be unaligned. */ +/* Return true if memory reference REF with step STEP may be unaligned. */ static bool may_be_unaligned_p (tree ref, tree step) { - tree base; - tree base_type; - HOST_WIDE_INT bitsize; - HOST_WIDE_INT bitpos; - tree toffset; - enum machine_mode mode; - int unsignedp, volatilep; - unsigned base_align; - /* TARGET_MEM_REFs are translated directly to valid MEMs on the target, thus they are not misaligned. */ if (TREE_CODE (ref) == TARGET_MEM_REF) return false; - /* The test below is basically copy of what expr.c:normal_inner_ref - does to check whether the object must be loaded by parts when - STRICT_ALIGNMENT is true. */ - base = get_inner_reference (ref, &bitsize, &bitpos, &toffset, &mode, - &unsignedp, &volatilep, true); - base_type = TREE_TYPE (base); - base_align = get_object_alignment (base); - base_align = MAX (base_align, TYPE_ALIGN (base_type)); - - if (mode != BLKmode) - { - unsigned mode_align = GET_MODE_ALIGNMENT (mode); - - if (base_align < mode_align - || (bitpos % mode_align) != 0 - || (bitpos % BITS_PER_UNIT) != 0) - return true; - - if (toffset - && (highest_pow2_factor (toffset) * BITS_PER_UNIT) < mode_align) - return true; + unsigned int align = TYPE_ALIGN (TREE_TYPE (ref)); - if ((highest_pow2_factor (step) * BITS_PER_UNIT) < mode_align) - return true; - } + unsigned HOST_WIDE_INT bitpos; + unsigned int ref_align; + get_object_alignment_1 (ref, &ref_align, &bitpos); + if (ref_align < align + || (bitpos % align) != 0 + || (bitpos % BITS_PER_UNIT) != 0) + return true; + + unsigned int trailing_zeros = tree_ctz (step); + if (trailing_zeros < HOST_BITS_PER_INT + && (1U << trailing_zeros) * BITS_PER_UNIT < align) + return true; return false; }