Re: [PATCH] testsuite: Robustify aarch64/simd tests against more aggressive DCE

[Andrew Pinski <pinskia@gmail.com>]

On Fri, Feb 4, 2022 at 3:21 AM Richard Sandiford via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Sorry, just realised I'd never replied to this.
>
> Marc Poulhies <poulhies@adacore.com> writes:
> > Eric Botcazou <botcazou@adacore.com> writes:
> >>> The new variables seem to be unused, so I think slightly stronger
> >>> DCE could remove the calls even after the patch.  Perhaps the containing
> >>> functions should take an int32x4_t *ptr or something, with the calls
> >>> assigning to different ptr[] indices.
> >>
> >> We run a minimal DCE pass at -O0 in our compiler to eliminate all the garbage
> >> generated by the gimplifier for variable-sized types (people care about code
> >> size at -O0 in specific contexts) but it does not touch anything written by
> >> the user (and debugging is unaffected of course).  Given that the builtins are
> >> pure functions and the arguments have no side effects, it eliminates the
> >> calls, but adding a LHS blocks that because this minimal DCE pass preserves
> >> anything user-related, in particular assignments to user variables.
> >>
> >>> I think it would be better to do that using new calls though,
> >>> and xfail the existing ones when they no longer work.  For example:
> >>>
> >>>   /* { dg-error "lane -1 out of range 0 - 7" "" {target *-*-*} 0 } */
> >>>   vqdmlal_high_laneq_s16 (int32x4_a, int16x8_b, int16x8_c, -1);
> >>>   /* { dg-error "lane -1 out of range 0 - 7" "" {target *-*-*} 0 } */
> >>>   ptr[0] = vqdmlal_high_laneq_s16 (int32x4_a, int16x8_b, int16x8_c, -1);
> >>>
> >>> That way we don't lose the existing tests.
> >>
> >> Frankly I'm not quite sure of what we can lose by adding a LHS here, can you
> >> elaborate a bit?  We would need a solution that works out of the box with our
> >> compiler in the future, i.e. without having to tweak 50 testcases again.
> >
> > Hi Richard,
> >
> > Thank for your reply !
> >
> > As Éric, I'm also wondering why having LHS in the existing tests would
> > make us loose them. I guess I'm not familiar enough with this part of
> > the testsuite and I'm missing something.
>
> The problem is that we only enforce lane bounds via calls to
> __builtin_aarch64_im_lane_boundsi.  In previous releases, the check
> only happend at RTL expansion time, so the check would be skipped if
> any gimple pass removed the call.  Now we do the checking during
> folding, but that still misses cases.  E.g., compare the -O0 and -O1
> behaviour for:

Actually I looked into the below testcase and
__builtin_aarch64_im_lane_boundsi is not part of the intrinsic.
Basically some intrinsics have their own bounds checking as part of
the builtin rather than using __builtin_aarch64_im_lane_boundsi.
That is the problem shows up in GCC 11 where the folding of
__builtin_aarch64_im_lane_boundsi on the gimple level didn't happen.
I will file a bug report on this regression later tonight or tomorrow.

Here are the uses of aarch64_simd_lane_bounds which emit the error
(besides the __builtin_aarch64_im_lane_boundsi builtin itself):

function:
aarch64_expand_fcmla_builtin

builtin_simd_arg args:
SIMD_ARG_STRUCT_LOAD_STORE_LANE_INDEX
SIMD_ARG_LANE_INDEX
SIMD_ARG_LANE_PAIR_INDEX
SIMD_ARG_LANE_QUADTUP_INDEX

rtl named patterns:
aarch64_ld<nregs>_lane<vstruct_elt>
aarch64_st<nregs>_lane<vstruct_elt>

Thanks,
Andrew Pinski

>
> #include <arm_neon.h>
>
> void f(int32x4_t *p0, int16x8_t *p1) {
>     vqdmlal_high_laneq_s16(p0[0], p1[0], p1[1], -1);
>     //p0[0] = vqdmlal_high_laneq_s16(p0[0], p1[0], p1[1], -1);
> }
>
> -O0 gives the error but -O1 doesn't [https://godbolt.org/z/1KosTY43T].
> The -O1 behaviour here is wrong: badly-formed calls should be rejected
> with a diagnostic even if the calls are unused.  Clang gets this right
> in both cases [https://godbolt.org/z/EGxs8jq97].
>
> I think keeping the lhs-free calls is important for making sure that
> the -O0 behaviour doesn't regress without the DCE.
>
> Your DCE will regress it, but that's the fault of the arm_neon.h
> implementation rather than the fault of your pass.  Having the
> tests but XFAILing them seems like the best way of dealing with that.
> Hopefully we'll then see some progression if the arm_neon.h implementation
> is improved in future.
>
> Thanks,
> Richard