On Wed, Jul 29, 2020 at 9:49 AM Stefan Schulze Frielinghaus <stefa...@linux.ibm.com> wrote: > > On Wed, Jul 29, 2020 at 09:11:12AM +0200, Richard Biener wrote: > > On Tue, Jul 28, 2020 at 5:36 PM Stefan Schulze Frielinghaus > > <stefa...@linux.ibm.com> wrote: > > > > > > On Tue, Jul 28, 2020 at 08:55:57AM +0200, Richard Biener wrote: > > > > On Mon, Jul 27, 2020 at 4:20 PM Stefan Schulze Frielinghaus > > > > <stefa...@linux.ibm.com> wrote: > > > > > > > > > > On Mon, Jul 27, 2020 at 12:29:11PM +0200, Richard Biener wrote: > > > > > > On Mon, Jul 27, 2020 at 11:45 AM Richard Sandiford > > > > > > <richard.sandif...@arm.com> wrote: > > > > > > > > > > > > > > Richard Biener <richard.guent...@gmail.com> writes: > > > > > > > > On Mon, Jul 27, 2020 at 11:09 AM Richard Sandiford > > > > > > > > <richard.sandif...@arm.com> wrote: > > > > > > > >> > > > > > > > >> Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> > > > > > > > >> writes: > > > > > > > >> > On Wed, Jul 22, 2020 at 5:18 PM Stefan Schulze Frielinghaus > > > > > > > >> > via > > > > > > > >> > Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > > > > > > >> >> > > > > > > > >> >> This is a follow up to commit 5c9669a0e6c respectively > > > > > > > >> >> discussion > > > > > > > >> >> https://gcc.gnu.org/pipermail/gcc-patches/2020-June/549132.html > > > > > > > >> >> > > > > > > > >> >> In case that an alignment constraint is less than the size > > > > > > > >> >> of a > > > > > > > >> >> corresponding scalar type, ensure that we advance at least > > > > > > > >> >> by one > > > > > > > >> >> iteration. For example, on s390x we have for a long double > > > > > > > >> >> an alignment > > > > > > > >> >> constraint of 8 bytes whereas the size is 16 bytes. > > > > > > > >> >> Therefore, > > > > > > > >> >> TARGET_ALIGN / DR_SIZE equals zero resulting in an infinite > > > > > > > >> >> loop which > > > > > > > >> >> can be reproduced by the following MWE: > > > > > > > >> > > > > > > > > >> > But we guard this case with vector_alignment_reachable_p, so > > > > > > > >> > we shouldn't > > > > > > > >> > have ended up here and the patch looks bogus. > > > > > > > >> > > > > > > > >> The above sounds like it ought to count as reachable alignment > > > > > > > >> though. > > > > > > > >> If a type requires a lower alignment than its size, then > > > > > > > >> that's even > > > > > > > >> more easily reachable than a type that requires the same > > > > > > > >> alignment as > > > > > > > >> the size. I guess at one extreme, a target alignment of 1 is > > > > > > > >> always > > > > > > > >> reachable. > > > > > > > > > > > > > > > > Well, if the element alignment is 8 but its size is 16 then > > > > > > > > when presumably > > > > > > > > the desired vector alignment is a multiple of 16 we can never > > > > > > > > reach it. > > > > > > > > Isn't this the case here? > > > > > > > > > > > > > > If the desired vector alignment (TARGET_ALIGN) is a multiple of > > > > > > > 16 then > > > > > > > TARGET_ALIGN / DR_SIZE will be nonzero and the problem the patch > > > > > > > is > > > > > > > fixing wouldn't occur. I agree that we might never be able to > > > > > > > reach > > > > > > > that alignment if the pointer starts out misaligned by 8 bytes. > > > > > > > > > > > > > > But I think that's why it makes sense for the target to only ask > > > > > > > for 8-byte alignment for vectors too, if it can cope with it. > > > > > > > 8-byte > > > > > > > alignment should always be achievable if the scalars are > > > > > > > ABI-aligned. > > > > > > > And if the target does ask for only 8-byte alignment, > > > > > > > TARGET_ALIGN / > > > > > > > DR_SIZE would be zero and the loop would never progress, which is > > > > > > > the > > > > > > > problem that the patch is fixing. > > > > > > > > > > > > > > It would even make sense for the target to ask for 1-byte > > > > > > > alignment, > > > > > > > if the target doesn't care about alignment at all. > > > > > > > > > > > > Hmm, OK. Guess I still think we should detect this somewhere upward > > > > > > and avoid this peeling compute at all. Somehow. > > > > > > > > > > I've been playing around with another solution which works for me by > > > > > changing vector_alignment_reachable_p to return also false if the > > > > > alignment requirements are already satisfied, i.e., by adding: > > > > > > > > > > if (known_alignment_for_access_p (dr_info) && aligned_access_p > > > > > (dr_info)) > > > > > return false; > > > > > > > > That sounds wrong, instead ... > > > > > > Can you elaborate on that? A similar test exists for predicate > > > vector_alignment_reachable_p where the second conjunct is the same but > > > negated in order to test for the case where a misalignment is known: > > > https://gcc.gnu.org/git?p=gcc.git;a=blob;f=gcc/tree-vect-data-refs.c;h=e35a215e042478d11d6545f1f829d816d0c3620f;hb=refs/heads/master#l1263 > > > Therefore, I'm wondering why the non-negated case should be wrong. > > > > > > > > Though, I'm not entirely sure whether this makes it better or not. > > > > > Strictly speaking if the alignment was reachable before peeling, then > > > > > reaching alignment with peeling is also possible but probably not what > > > > > was intended. So I guess returning false in this case is sensible. > > > > > Any > > > > > comments? > > > > > > > > ... why is the DR considered for peeling at all? If it is already > > > > aligned there's > > > > no point to do that. > > > > > > Isn't the whole point of vector_alignment_reachable_p to check DRs in > > > order to decide whether peeling should be done or not? At least this is > > > my intuition and the reason why I was suggesting to return false in case > > > it is aligned. > > > > Doh, you are right - I confused the function to be a mere wrapper > > around the VECTOR_ALIGNMENT_REACHABLE target hook. But > > yes, it's exactly what you say. But with your suggested extra check > > the code at the point of the call would simply disable peeling? The > > code looks odd anyway - it does > > > > FOR_EACH_VEC_ELT (datarefs, i, dr) > > { > > ... > > do_peeling = vector_alignment_reachable_p (dr_info); > > if (do_peeling) > > { > > ... insert into peeling hash for costing - also inserts already aligned > > accesses which may get unaligned with peeling > > } > > else > > { > > if (!aligned_access_p (dr_info)) > > { > > if (dump_enabled_p ()) > > dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location, > > "vector alignment may not be reachable\n"); > > break; > > } > > } > > } > > > > so in your case when do_peeling is false we'll not keep it false because > > aligned_access_p () and then the next DR might make do_peeling true > > again which will simply cause your rejected DR to be not considered for > > costing. So I think in the else {} case the aligned_access_p () case > > is broken already and your proposal makes it more likely to hit. Not > > sure if we'd currently survive turning that if (!aligned_access_p ()) > > into an assert ... > > > > In that light your original patch looks correct. > > Whoopsy, yes, I forgot to consider a rejected DR for costing in my > second try. The longer I stare at the code the more I tend to the > original patch. Thus if no one objects I would like to commit the > original patch.
Yes please! > Thanks for taking a close look at it! Sorry for contributing to the confusion ;) Richard. > Cheers, > Stefan > > > > > Thanks, > > Richard. > > > > > Cheers, > > > Stefan > > > > > > > If we want to align another DR then the loop you fix > > > > should run on that DRs align/size, no? > > > > > > > > Richard. > > > > > > > > > Thanks, > > > > > Stefan > > > > > > > > > > > > > > > > > Richard. > > > > > > > > > > > > > Thanks, > > > > > > > Richard
