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 <poulh...@adacore.com> writes:
> > Eric Botcazou <botca...@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
