On Fri, 6 Dec 2024 at 12:41, Richard Earnshaw (lists) <richard.earns...@arm.com> wrote: > > On 04/12/2024 20:56, Christophe Lyon wrote: > > On Wed, 4 Dec 2024 at 12:39, Richard Earnshaw (lists) > > <richard.earns...@arm.com> wrote: > >> > >> On 25/11/2024 20:08, Christophe Lyon wrote: > >>> In this PR, we have to handle a case where MVE predicates are supplied > >>> as a const_int, where individual predicates have illegal boolean > >>> values (such as 0xc for a 4-bit boolean predicate). To avoid the ICE, > >>> fix the constant (any non-zero value is converted to all 1s) and emit > >>> a warning. > >>> > >>> On MVE, V8BI and V4BI multi-bit masks are interpreted byte-by-byte at > >>> instruction level, but end-users should describe lanes rather than > >>> bytes (so all bytes of a true-predicated lane should be '1'), see > >>> https://developer.arm.com/documentation/101028/0012/14--M-profile-Vector-Extension--MVE--intrinsics. > >>> > >>> Since gen_lowpart can ICE on a subreg, we force predicates in a subreg > >>> into a reg, after removing subreg of the same size as the target > >>> (HImode) which would be made redundant by gen_lowpart and confuse the > >>> DLSTP optimization. > >>> > >>> 2024-11-20 Christophe Lyon <christophe.l...@linaro.org> > >>> Jakub Jelinek <ja...@redhat.com> > >>> > >>> PR target/114801 > >>> gcc/ > >>> * config/arm/arm-mve-builtins.cc > >>> (function_expander::add_input_operand): Handle CONST_INT > >>> predicates. > >>> > >>> gcc/testsuite/ > >>> * gcc.target/arm/mve/pr108443.c: Update predicate constant. > >>> * gcc.target/arm/mve/pr114801.c: New test. > >>> --- > >>> gcc/config/arm/arm-mve-builtins.cc | 37 ++++++++++++++++++- > >>> gcc/testsuite/gcc.target/arm/mve/pr108443.c | 4 +-- > >>> gcc/testsuite/gcc.target/arm/mve/pr114801.c | 39 +++++++++++++++++++++ > >>> 3 files changed, 77 insertions(+), 3 deletions(-) > >>> create mode 100644 gcc/testsuite/gcc.target/arm/mve/pr114801.c > >>> > >>> diff --git a/gcc/config/arm/arm-mve-builtins.cc > >>> b/gcc/config/arm/arm-mve-builtins.cc > >>> index 255aed25600..5ff32ce06b7 100644 > >>> --- a/gcc/config/arm/arm-mve-builtins.cc > >>> +++ b/gcc/config/arm/arm-mve-builtins.cc > >>> @@ -2352,7 +2352,42 @@ function_expander::add_input_operand (insn_code > >>> icode, rtx x) > >>> mode = GET_MODE (x); > >>> } > >>> else if (VALID_MVE_PRED_MODE (mode)) > >>> - x = gen_lowpart (mode, x); > >>> + { > >>> + if (CONST_INT_P (x) && (mode == V8BImode || mode == V4BImode)) > >>> + { > >>> + /* In V8BI or V4BI each element has 2 or 4 bits, if those bits > >>> aren't > >>> + all the same, gen_lowpart might ICE. Canonicalize all the 2 > >>> or 4 > >>> + bits to all ones if any of them is non-zero. V8BI and V4BI > >>> + multi-bit masks are interpreted byte-by-byte at instruction > >>> level, > >>> + but such constants should describe lanes, rather than bytes. > >>> See > >>> + > >>> https://developer.arm.com/documentation/101028/0012/14--M-profile-Vector-Extension--MVE--intrinsics. > >>> */ > >> > >> Apart from being an overly long line, deep links like this are generally > >> not very stable. I suggest we just say something like "See the section on > >> MVE intrinsics in the Arm ACLE specification". > > > > Right, I was wondering what was the best practice, I think I've seen > > such links recently, not sure where. > > I'll update the comment, and the commit message. > > > >> > >>> + unsigned HOST_WIDE_INT xi = UINTVAL (x); > >>> + xi |= ((xi & 0x5555) << 1) | ((xi & 0xaaaa) >> 1); > >>> + if (mode == V4BImode) > >>> + xi |= ((xi & 0x3333) << 2) | ((xi & 0xcccc) >> 2); > >>> + if (xi != UINTVAL (x)) > >>> + inform (location, "constant predicate argument %d (%wx) does" > >>> + " not map to %d lane numbers, converted to %wx", > >>> + opno, UINTVAL (x) & 0xffff, mode == V8BImode ? 8 : 4, > >>> + xi & 0xffff); > >> > >> I think this should be a warning (so that werror can work with it). > >> Otherwise such messages can't be faulted. > > OK, I will change this. > > > >> > >>> + > >>> + x = gen_int_mode (xi, HImode); > >>> + } > >>> + else if (SUBREG_P (x)) > >>> + { > >>> + /* Already of the right size, drop the subreg which will be made > >>> + redundant by gen_lowpart below. */ > >>> + if (GET_MODE_SIZE (GET_MODE (x)) == GET_MODE_SIZE (HImode) > >>> + || SUBREG_BYTE (x) == 0) > >>> + x = SUBREG_REG (x); > >>> + > >>> + /* gen_lowpart on a SUBREG can ICE. */ > >>> + if (gen_lowpart_common (mode, x) == 0) > >>> + x = force_reg (GET_MODE (x), x); > >>> + } > >>> + > >>> + x = gen_lowpart (mode, x); > >> > >> I wonder if this is overly complex. Wouldn't it be better to just write > >> here: > >> > >> else if (!REG_P (x)) > >> x = force_reg (GET_MODE (x), x); > >> > >> and then let the optimizers clean things up? > >> > > The !REG_P(x) condition is not right, because force_reg crashes if x > > == (const_int 1) for instance (we have mode=VOID in > > int_mode_for_mode). > > > > The first 'if' which looks at the mode sizes it to avoid regressions > > in DLSTP transform, despite recent improvements there. > > Without this, dlstp-compile-asm-2.c has regressions in test6 and test9. > > For some reason we have: > > (set (reg:HI 137 [ p1 ] > > (subreg/s/v:HI (reg/v:SI 129 [ p1 ]) 0)) (nil)) > > (set (reg:V4BI 138 [ p1 ]) > > (subreg:V4BI (reg:HI 137 [ p1 ]) 0)) (nil)) > > which not hoisted, while with the check above we hoist this: > > (set (reg:V4BI 136 [ p1 ]) > > (subreg:V4BI (reg/v:SI 129 [ p1 ]) 0)) (nil)) > > > > I was thinking this could be OK for gcc-15, and I would look at > > removing this hack for gcc-16. > > > > The gen_lowpart_common (mode, x) == 0 part is to call force_reg only > > if gen_lowpart might have trouble with the current x, but avoid > > unnecessary force_reg that could confuse DLSTP transform too. > > In practice this is not exercised by the testsuite, but Jakub raised > > concerns after gen_lowpart possibly ICEing. > > > > So how about using force_subreg_lowpart at the end, rather than gen_lowpart? Do you mean gen_lowpart_SUBREG? (force_subreg_lowpart does not exist?)
gen_lowpart_SUBREG ICEs when x is a const_int, so do you mean: if (CONST_INT_P (x)) x = gen_lowpart (mode, x); else x = gen_lowpart_SUBREG (mode, x); Without the checks of the mode sizes, we still have regressions in DLSTP transform anyway. Thanks, Christophe > > R. > > Thanks, > > > > Christophe > > > >> R. > >> >
