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? R. > Thanks, > > Christophe > >> R. >>
