On Tue, Sep 29, 2026 at 3:32 PM Torbjorn SVENSSON <[email protected]> wrote: > > > > On 2026-09-29 14:57, Richard Biener wrote: > > On Tue, Sep 29, 2026 at 9:52 AM Torbjorn SVENSSON > > <[email protected]> wrote: > >> > >> > >> > >> On 2026-09-24 15:56, Richard Biener wrote: > >>> On Thu, Sep 24, 2026 at 3:34 PM Torbjorn SVENSSON > >>> <[email protected]> wrote: > >>>> > >>>> > >>>> > >>>> On 2026-09-24 15:25, Richard Biener wrote: > >>>>> On Thu, Sep 24, 2026 at 2:24 PM Torbjorn SVENSSON > >>>>> <[email protected]> wrote: > >>>>>> > >>>>>> > >>>>>> > >>>>>> On 2026-09-23 15:40, Richard Biener wrote: > >>>>>>> On Wed, Sep 2, 2026 at 1:18 PM Torbjörn Svensson via Sourceware Forge > >>>>>>> <[email protected]> wrote: > >>>>>>>> > >>>>>>>> From: Torbjörn SVENSSON <[email protected]> > >>>>>>>> > >>>>>>>> vect_slp_v2qi_store_unalign tests whether the vectorizer handles a > >>>>>>>> V2QI > >>>>>>>> store through a byte-aligned base, but some -Wstringop-overflow tests > >>>>>>>> instead depend on a V2QI store at a known one-byte misalignment from > >>>>>>>> a > >>>>>>>> defined object. > >>>>>>>> > >>>>>>>> These two variants are not the same on armv7ve with -mfloat-abi=hard. > >>>>>>>> Add a separate effective target for the latter case. > >>>>>>> > >>>>>>> Can you again explain why they are not the same? I'm guessing it's > >>>>>>> about knowing the misalignment vs. unknown misalignment but then > >>>>>>> vect_slp_v2qi_store_misalign vs. _unalign is not obvious. Maybe > >>>>>>> _unknown_misalign vs. _known_misalign? But I'm still curious > >>>>>>> why a target choses to allow one but not the other (for stores there's > >>>>>>> no dr_explicit_realign[_optimized])? > >>>>>> > >>>>>> Unless I've completly missunderstood the sequence, it should stem from > >>>>>> https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/arm/arm.cc;hb=refs/heads/trunk#l30984 > >>>>>> > >>>>>> After another bisect, I identified that the first failing commit is > >>>>>> r16-4314-g5e9eecc6686 (for r16-4315 and later, r16-4355-g53085a46e7f162 > >>>>>> is also needed to build). > >>>>>> > >>>>>> I think the reason for the regression is that the value of "align" is > >>>>>> not the same for TYPE_ALIGN_UNIT(type) and GET_MODE_UNIT_SIZE(mode). > >>>>>> > >>>>>> In the v2qi tests, mode would match HImode, thus: > >>>>>> > >>>>>> - GET_MODE_UNIT_SIZE(HImode) would return 2. > >>>>>> - TYPE_ALIGN_UNIT(type) would return 1 for char. > >>>>>> > >>>>>> So, maybe my patch is not correct and instead the function should > >>>>>> simply be like this since NEON apparently accepts unknown alignment: > >>>>>> > >>>>>> static bool > >>>>>> arm_builtin_support_vector_misalignment (machine_mode mode, > >>>>>> int misalignment, > >>>>>> bool is_packed, > >>>>>> bool is_gather_scatter) > >>>>>> { > >>>>>> if (TARGET_NEON && !BYTES_BIG_ENDIAN && unaligned_access) > >>>>>> { > >>>>>> HOST_WIDE_INT align = GET_MODE_UNIT_SIZE (mode); > >>>>>> > >>>>>> if (is_gather_scatter) > >>>>>> return true; > >>>>>> > >>>>>> if (is_packed) > >>>>>> return align == 1; > >>>>>> > >>>>>> /* NEON supports access regardless of the alignment. */ > >>>>>> return true; > >>>>>> } > >>>>>> > >>>>>> return default_builtin_support_vector_misalignment (mode, > >>>>>> misalignment, > >>>>>> is_packed, > >>>>>> > >>>>>> is_gather_scatter); > >>>>>> } > >>>>>> > >>>>>> > >>>>>> WDYT? > >>>>> > >>>>> Yeah, the current code is > >>>>> > >>>>> /* If the misalignment is unknown, we should be able to handle > >>>>> the access > >>>>> so long as it is not to a member of a packed data > >>>>> structure. */ > >>>>> if (misalignment == -1) > >>>>> return true; > >>>>> > >>>>> /* Return true if the misalignment is a multiple of the > >>>>> natural alignment > >>>>> of the vector's element type. This is probably always > >>>>> going to be > >>>>> true in practice, since we've already established that this > >>>>> isn't a > >>>>> packed access. */ > >>>>> return ((misalignment % align) == 0); > >>>>> > >>>>> which says it accepts any misalignment (misalignment == -1) but not > >>>>> a known misalignment, of say 3. That's just odd. > >>>>> > >>>>>> Also, the check "align == 1" in the is_packed branch is probably not > >>>>>> doing the right thing since the value in "align" is the size(?) and not > >>>>>> the alignment. > >>>>> > >>>>> yeah, a vector mode of size 1 probably does not exist. Iff we > >>>>> accept misalignment == -1 there's no reason to reject "is_packed". > >>>>> is_packed just means we cannot reach alignment by peeling. > >>>>> There's no good reason to check is_packed. > >>>> > >>>> So, based on this, maybe the function body could be reduced to: > >>>> > >>>> { > >>>> if (TARGET_NEON && !BYTES_BIG_ENDIAN && unaligned_access) > >>>> return true; > >>>> > >>>> return default_builtin_support_vector_misalignment (mode, > >>>> misalignment, > >>>> is_packed, > >>>> > >>>> is_gather_scatter); > >>>> } > >>>> > >>>> I can do this change and do a regression test run on it to see if it > >>>> causes > >>>> any regressions if you think this is better. > >>> > >>> Looks reasonable. Note the aarch64 variant does > >>> > >>> if (TARGET_SIMD && STRICT_ALIGNMENT) > >>> { > >>> if (is_gather_scatter) > >>> return true; > >>> > >>> /* Return if movmisalign pattern is not supported for this mode. > >>> */ > >>> if (optab_handler (movmisalign_optab, mode) == CODE_FOR_nothing) > >>> return false; > >>> > >>> /* Misalignment factor is unknown at compile time. */ > >>> if (misalignment == -1) > >>> return false; > >>> } > >>> return default_builtin_support_vector_misalignment (mode, > >>> misalignment, > >>> is_packed, > >>> > >>> is_gather_scatter); > >>> > >>> so basically defaults for !STRICT_ALIGNMENT. I suppose the equivalent > >>> is unaligned_access > >>> on the arm side. Compared to your variant with 'return true' it falls > >>> through to the > >>> default (which tests for movmisalign support of the mode). So if arm > >>> can do that for all > >>> vector modes a return true sounds good, if not, fallthru to the > >>> default hook looks safer. > >> > >> I tried to use the aarch64 approach, but replaced TARGET_SIMD with > >> TARGET_NEON. > >> This works for the test in question, but it causes other regressions for > >> the > >> vect-early-break. > >> Then I tried to simply return true for any TARGET_NEON, same result. > >> > >> After having tried all these ways, I got lazy and simply asked AI if it was > >> possible to solve this without having the "type" parameter and the > >> conclusion > >> it gave me was: > >> > >> There is no generic way for this hook to exactly reproduce > >> `TYPE_ALIGN_UNIT(type)` from only `mode`, `misalignment`, and the flags: > >> two > >> accesses can have the same hook arguments but different element-type > >> alignment. > >> The broad NEON `return true` passes `Wstringop-overflow-2.c` but adds four > >> `vect-early-break` failures. The mode-unit rule with an HImode adjustment > >> still > >> adds those failures; the full comparison is also affected by different > >> non-GCC > >> components in the prebuilt baseline versus this build. > >> > >> > >> I think it's safe to say that this might be an unsolvable problem. > > > > But what matters for the target is the mode, not the type. So IMO the > > AI is misleading > > here or the vectorizer asks for a non-natural aligned vector type > > which it should not. > > I'm not so worried about the diagnostic part of the failure, but I presume > that this > function also controls what code that might be generated. Thus, this could > potentially > lead to wrong code or suboptimal code. > Obvious, I would like the failure to go away since it's about 160 fail/xpass > for these > 2 tests. > > What would you say is the next step to figure out what is going wrong here? > > In r16-4314-g5e9eecc6686, I see that riscv might also have a similar problem > based on > this change: > > { > /* If we know that misalignment is a multiple of the element size, > we're > good. */ > - if (misalignment % TYPE_ALIGN_UNIT (type) == 0) > + if (misalignment % (GET_MODE_UNIT_SIZE (mode)) == 0) > return true; > } > > Have you heard anyone in the riscv gang that noticed a problem here?
Oh, but that is changing misalignment % alignment for misalignment % size (which is more conservative). I'd expect TYPE_ALIGN and GET_MODE_ALIGNMENT to agree (also size!) and if you put an assert to that near the single(?) caller of the hook that should never fire. Does it for the testcases you point out regress? > > > Kind regards, > Torbjörn
