| Message ID | Zbk1a29088XKSBtA@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [13] aarch64: Avoid out-of-range shrink-wrapped saves [PR111677] | expand |
Alex Coplan <alex.coplan@arm.com> writes: > Bootstrapped/regtested on aarch64-linux-gnu, OK for the 13 branch after > a week of the trunk fix being in? OK for the other active branches if > the same changes test cleanly there? > > GCC 14 patch for reference: > https://gcc.gnu.org/pipermail/gcc-patches/2024-January/644441.html > > Thanks, > Alex OK, thanks. It might be worth committing sooner than that, since the patch includes changes that won't be tested on trunk. It's your call though. Richard > > -- >8 -- > > The PR shows us ICEing due to an unrecognizable TFmode save emitted by > aarch64_process_components. The problem is that for T{I,F,D}mode we > conservatively require mems to be in range for x-register ldp/stp. That > is because (at least for TImode) it can be allocated to both GPRs and > FPRs, and in the GPR case that is an x-reg ldp/stp, and the FPR case is > a q-register load/store. > > As Richard pointed out in the PR, aarch64_get_separate_components > already checks that the offsets are suitable for a single load, so we > just need to choose a mode in aarch64_reg_save_mode that gives the full > q-register range. In this patch, we choose V16QImode as an alternative > 16-byte "bag-of-bits" mode that doesn't have the artificial range > restrictions imposed on T{I,F,D}mode. > > For T{F,D}mode in GCC 15 I think we could consider relaxing the > restriction imposed in aarch64_classify_address, as AFAIK T{F,D}mode can > only be allocated to FPRs (unlike TImode). But such a change seems too > invasive to consider for GCC 14 at this stage (let alone backports). > > Unlike for GCC 14 we need additional handling in the load/store pair > code as various cases are not expecting to see V16QImode (particularly > the writeback patterns, but also aarch64_gen_load_pair). > > gcc/ChangeLog: > > PR target/111677 > * config/aarch64/aarch64.cc (aarch64_reg_save_mode): Use > V16QImode for the full 16-byte FPR saves in the vector PCS case. > (aarch64_gen_storewb_pair): Handle V16QImode. > (aarch64_gen_loadwb_pair): Likewise. > (aarch64_gen_load_pair): Likewise. > * config/aarch64/aarch64.md (loadwb_pair<TX:mode>_<P:mode>): > Rename to ... > (loadwb_pair<TX_V16QI:mode>_<P:mode>): ... this, extending to > V16QImode. > (storewb_pair<TX:mode>_<P:mode>): Rename to ... > (storewb_pair<TX_V16QI:mode>_<P:mode>): ... this, extending to > V16QImode. > * config/aarch64/iterators.md (TX_V16QI): New. > > gcc/testsuite/ChangeLog: > > PR target/111677 > * gcc.target/aarch64/torture/pr111677.c: New test. > > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc > index 02515d4683a..f546c48ae2d 100644 > --- a/gcc/config/aarch64/aarch64.cc > +++ b/gcc/config/aarch64/aarch64.cc > @@ -4074,7 +4074,7 @@ aarch64_reg_save_mode (unsigned int regno) > case ARM_PCS_SIMD: > /* The vector PCS saves the low 128 bits (which is the full > register on non-SVE targets). */ > - return TFmode; > + return V16QImode; > > case ARM_PCS_SVE: > /* Use vectors of DImode for registers that need frame > @@ -8863,6 +8863,10 @@ aarch64_gen_storewb_pair (machine_mode mode, rtx base, rtx reg, rtx reg2, > return gen_storewb_pairtf_di (base, base, reg, reg2, > GEN_INT (-adjustment), > GEN_INT (UNITS_PER_VREG - adjustment)); > + case E_V16QImode: > + return gen_storewb_pairv16qi_di (base, base, reg, reg2, > + GEN_INT (-adjustment), > + GEN_INT (UNITS_PER_VREG - adjustment)); > default: > gcc_unreachable (); > } > @@ -8908,6 +8912,10 @@ aarch64_gen_loadwb_pair (machine_mode mode, rtx base, rtx reg, rtx reg2, > case E_TFmode: > return gen_loadwb_pairtf_di (base, base, reg, reg2, GEN_INT (adjustment), > GEN_INT (UNITS_PER_VREG)); > + case E_V16QImode: > + return gen_loadwb_pairv16qi_di (base, base, reg, reg2, > + GEN_INT (adjustment), > + GEN_INT (UNITS_PER_VREG)); > default: > gcc_unreachable (); > } > @@ -8991,6 +8999,9 @@ aarch64_gen_load_pair (machine_mode mode, rtx reg1, rtx mem1, rtx reg2, > case E_V4SImode: > return gen_load_pairv4siv4si (reg1, mem1, reg2, mem2); > > + case E_V16QImode: > + return gen_load_pairv16qiv16qi (reg1, mem1, reg2, mem2); > + > default: > gcc_unreachable (); > } > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md > index 50239d72fc0..922cc987595 100644 > --- a/gcc/config/aarch64/aarch64.md > +++ b/gcc/config/aarch64/aarch64.md > @@ -1896,17 +1896,18 @@ (define_insn "loadwb_pair<GPF:mode>_<P:mode>" > [(set_attr "type" "neon_load1_2reg")] > ) > > -(define_insn "loadwb_pair<TX:mode>_<P:mode>" > +(define_insn "loadwb_pair<TX_V16QI:mode>_<P:mode>" > [(parallel > [(set (match_operand:P 0 "register_operand" "=k") > - (plus:P (match_operand:P 1 "register_operand" "0") > - (match_operand:P 4 "aarch64_mem_pair_offset" "n"))) > - (set (match_operand:TX 2 "register_operand" "=w") > - (mem:TX (match_dup 1))) > - (set (match_operand:TX 3 "register_operand" "=w") > - (mem:TX (plus:P (match_dup 1) > + (plus:P (match_operand:P 1 "register_operand" "0") > + (match_operand:P 4 "aarch64_mem_pair_offset" "n"))) > + (set (match_operand:TX_V16QI 2 "register_operand" "=w") > + (mem:TX_V16QI (match_dup 1))) > + (set (match_operand:TX_V16QI 3 "register_operand" "=w") > + (mem:TX_V16QI (plus:P (match_dup 1) > (match_operand:P 5 "const_int_operand" "n"))))])] > - "TARGET_SIMD && INTVAL (operands[5]) == GET_MODE_SIZE (<TX:MODE>mode)" > + "TARGET_SIMD > + && known_eq (INTVAL (operands[5]), GET_MODE_SIZE (<TX_V16QI:MODE>mode))" > "ldp\\t%q2, %q3, [%1], %4" > [(set_attr "type" "neon_ldp_q")] > ) > @@ -1945,20 +1946,20 @@ (define_insn "storewb_pair<GPF:mode>_<P:mode>" > [(set_attr "type" "neon_store1_2reg<q>")] > ) > > -(define_insn "storewb_pair<TX:mode>_<P:mode>" > +(define_insn "storewb_pair<TX_V16QI:mode>_<P:mode>" > [(parallel > [(set (match_operand:P 0 "register_operand" "=&k") > - (plus:P (match_operand:P 1 "register_operand" "0") > - (match_operand:P 4 "aarch64_mem_pair_offset" "n"))) > - (set (mem:TX (plus:P (match_dup 0) > + (plus:P (match_operand:P 1 "register_operand" "0") > + (match_operand:P 4 "aarch64_mem_pair_offset" "n"))) > + (set (mem:TX_V16QI (plus:P (match_dup 0) > (match_dup 4))) > - (match_operand:TX 2 "register_operand" "w")) > - (set (mem:TX (plus:P (match_dup 0) > + (match_operand:TX_V16QI 2 "register_operand" "w")) > + (set (mem:TX_V16QI (plus:P (match_dup 0) > (match_operand:P 5 "const_int_operand" "n"))) > - (match_operand:TX 3 "register_operand" "w"))])] > + (match_operand:TX_V16QI 3 "register_operand" "w"))])] > "TARGET_SIMD > - && INTVAL (operands[5]) > - == INTVAL (operands[4]) + GET_MODE_SIZE (<TX:MODE>mode)" > + && known_eq (INTVAL (operands[5]), > + INTVAL (operands[4]) + GET_MODE_SIZE (<TX_V16QI:MODE>mode))" > "stp\\t%q2, %q3, [%0, %4]!" > [(set_attr "type" "neon_stp_q")] > ) > diff --git a/gcc/config/aarch64/iterators.md b/gcc/config/aarch64/iterators.md > index 6cbc97cc82c..86a196d3536 100644 > --- a/gcc/config/aarch64/iterators.md > +++ b/gcc/config/aarch64/iterators.md > @@ -313,6 +313,9 @@ (define_mode_iterator VS [V2SI V4SI]) > > (define_mode_iterator TX [TI TF TD]) > > +;; TX plus V16QImode. > +(define_mode_iterator TX_V16QI [TI TF TD V16QI]) > + > (define_mode_iterator VTX [TI TF TD V16QI V8HI V4SI V2DI V8HF V4SF V2DF V8BF]) > > ;; Advanced SIMD opaque structure modes. > diff --git a/gcc/testsuite/gcc.target/aarch64/torture/pr111677.c b/gcc/testsuite/gcc.target/aarch64/torture/pr111677.c > new file mode 100644 > index 00000000000..6bb640c42c0 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/torture/pr111677.c > @@ -0,0 +1,28 @@ > +/* { dg-do compile } */ > +/* { dg-require-effective-target fopenmp } */ > +/* { dg-options "-ffast-math -fstack-protector-strong -fopenmp" } */ > +typedef struct { > + long size_z; > + int width; > +} dt_bilateral_t; > +typedef float dt_aligned_pixel_t[4]; > +#pragma omp declare simd > +void dt_bilateral_splat(dt_bilateral_t *b) { > + float *buf; > + long offsets[8]; > + for (; b;) { > + int firstrow; > + for (int j = firstrow; j; j++) > + for (int i; i < b->width; i++) { > + dt_aligned_pixel_t contrib; > + for (int k = 0; k < 4; k++) > + buf[offsets[k]] += contrib[k]; > + } > + float *dest; > + for (int j = (long)b; j; j++) { > + float *src = (float *)b->size_z; > + for (int i = 0; i < (long)b; i++) > + dest[i] += src[i]; > + } > + } > +}
diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc index 02515d4683a..f546c48ae2d 100644 --- a/gcc/config/aarch64/aarch64.cc +++ b/gcc/config/aarch64/aarch64.cc @@ -4074,7 +4074,7 @@ aarch64_reg_save_mode (unsigned int regno) case ARM_PCS_SIMD: /* The vector PCS saves the low 128 bits (which is the full register on non-SVE targets). */ - return TFmode; + return V16QImode; case ARM_PCS_SVE: /* Use vectors of DImode for registers that need frame @@ -8863,6 +8863,10 @@ aarch64_gen_storewb_pair (machine_mode mode, rtx base, rtx reg, rtx reg2, return gen_storewb_pairtf_di (base, base, reg, reg2, GEN_INT (-adjustment), GEN_INT (UNITS_PER_VREG - adjustment)); + case E_V16QImode: + return gen_storewb_pairv16qi_di (base, base, reg, reg2, + GEN_INT (-adjustment), + GEN_INT (UNITS_PER_VREG - adjustment)); default: gcc_unreachable (); } @@ -8908,6 +8912,10 @@ aarch64_gen_loadwb_pair (machine_mode mode, rtx base, rtx reg, rtx reg2, case E_TFmode: return gen_loadwb_pairtf_di (base, base, reg, reg2, GEN_INT (adjustment), GEN_INT (UNITS_PER_VREG)); + case E_V16QImode: + return gen_loadwb_pairv16qi_di (base, base, reg, reg2, + GEN_INT (adjustment), + GEN_INT (UNITS_PER_VREG)); default: gcc_unreachable (); } @@ -8991,6 +8999,9 @@ aarch64_gen_load_pair (machine_mode mode, rtx reg1, rtx mem1, rtx reg2, case E_V4SImode: return gen_load_pairv4siv4si (reg1, mem1, reg2, mem2); + case E_V16QImode: + return gen_load_pairv16qiv16qi (reg1, mem1, reg2, mem2); + default: gcc_unreachable (); } diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index 50239d72fc0..922cc987595 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -1896,17 +1896,18 @@ (define_insn "loadwb_pair<GPF:mode>_<P:mode>" [(set_attr "type" "neon_load1_2reg")] ) -(define_insn "loadwb_pair<TX:mode>_<P:mode>" +(define_insn "loadwb_pair<TX_V16QI:mode>_<P:mode>" [(parallel [(set (match_operand:P 0 "register_operand" "=k") - (plus:P (match_operand:P 1 "register_operand" "0") - (match_operand:P 4 "aarch64_mem_pair_offset" "n"))) - (set (match_operand:TX 2 "register_operand" "=w") - (mem:TX (match_dup 1))) - (set (match_operand:TX 3 "register_operand" "=w") - (mem:TX (plus:P (match_dup 1) + (plus:P (match_operand:P 1 "register_operand" "0") + (match_operand:P 4 "aarch64_mem_pair_offset" "n"))) + (set (match_operand:TX_V16QI 2 "register_operand" "=w") + (mem:TX_V16QI (match_dup 1))) + (set (match_operand:TX_V16QI 3 "register_operand" "=w") + (mem:TX_V16QI (plus:P (match_dup 1) (match_operand:P 5 "const_int_operand" "n"))))])] - "TARGET_SIMD && INTVAL (operands[5]) == GET_MODE_SIZE (<TX:MODE>mode)" + "TARGET_SIMD + && known_eq (INTVAL (operands[5]), GET_MODE_SIZE (<TX_V16QI:MODE>mode))" "ldp\\t%q2, %q3, [%1], %4" [(set_attr "type" "neon_ldp_q")] ) @@ -1945,20 +1946,20 @@ (define_insn "storewb_pair<GPF:mode>_<P:mode>" [(set_attr "type" "neon_store1_2reg<q>")] ) -(define_insn "storewb_pair<TX:mode>_<P:mode>" +(define_insn "storewb_pair<TX_V16QI:mode>_<P:mode>" [(parallel [(set (match_operand:P 0 "register_operand" "=&k") - (plus:P (match_operand:P 1 "register_operand" "0") - (match_operand:P 4 "aarch64_mem_pair_offset" "n"))) - (set (mem:TX (plus:P (match_dup 0) + (plus:P (match_operand:P 1 "register_operand" "0") + (match_operand:P 4 "aarch64_mem_pair_offset" "n"))) + (set (mem:TX_V16QI (plus:P (match_dup 0) (match_dup 4))) - (match_operand:TX 2 "register_operand" "w")) - (set (mem:TX (plus:P (match_dup 0) + (match_operand:TX_V16QI 2 "register_operand" "w")) + (set (mem:TX_V16QI (plus:P (match_dup 0) (match_operand:P 5 "const_int_operand" "n"))) - (match_operand:TX 3 "register_operand" "w"))])] + (match_operand:TX_V16QI 3 "register_operand" "w"))])] "TARGET_SIMD - && INTVAL (operands[5]) - == INTVAL (operands[4]) + GET_MODE_SIZE (<TX:MODE>mode)" + && known_eq (INTVAL (operands[5]), + INTVAL (operands[4]) + GET_MODE_SIZE (<TX_V16QI:MODE>mode))" "stp\\t%q2, %q3, [%0, %4]!" [(set_attr "type" "neon_stp_q")] ) diff --git a/gcc/config/aarch64/iterators.md b/gcc/config/aarch64/iterators.md index 6cbc97cc82c..86a196d3536 100644 --- a/gcc/config/aarch64/iterators.md +++ b/gcc/config/aarch64/iterators.md @@ -313,6 +313,9 @@ (define_mode_iterator VS [V2SI V4SI]) (define_mode_iterator TX [TI TF TD]) +;; TX plus V16QImode. +(define_mode_iterator TX_V16QI [TI TF TD V16QI]) + (define_mode_iterator VTX [TI TF TD V16QI V8HI V4SI V2DI V8HF V4SF V2DF V8BF]) ;; Advanced SIMD opaque structure modes. diff --git a/gcc/testsuite/gcc.target/aarch64/torture/pr111677.c b/gcc/testsuite/gcc.target/aarch64/torture/pr111677.c new file mode 100644 index 00000000000..6bb640c42c0 --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/torture/pr111677.c @@ -0,0 +1,28 @@ +/* { dg-do compile } */ +/* { dg-require-effective-target fopenmp } */ +/* { dg-options "-ffast-math -fstack-protector-strong -fopenmp" } */ +typedef struct { + long size_z; + int width; +} dt_bilateral_t; +typedef float dt_aligned_pixel_t[4]; +#pragma omp declare simd +void dt_bilateral_splat(dt_bilateral_t *b) { + float *buf; + long offsets[8]; + for (; b;) { + int firstrow; + for (int j = firstrow; j; j++) + for (int i; i < b->width; i++) { + dt_aligned_pixel_t contrib; + for (int k = 0; k < 4; k++) + buf[offsets[k]] += contrib[k]; + } + float *dest; + for (int j = (long)b; j; j++) { + float *src = (float *)b->size_z; + for (int i = 0; i < (long)b; i++) + dest[i] += src[i]; + } + } +}
Bootstrapped/regtested on aarch64-linux-gnu, OK for the 13 branch after a week of the trunk fix being in? OK for the other active branches if the same changes test cleanly there? GCC 14 patch for reference: https://gcc.gnu.org/pipermail/gcc-patches/2024-January/644441.html Thanks, Alex -- >8 -- The PR shows us ICEing due to an unrecognizable TFmode save emitted by aarch64_process_components. The problem is that for T{I,F,D}mode we conservatively require mems to be in range for x-register ldp/stp. That is because (at least for TImode) it can be allocated to both GPRs and FPRs, and in the GPR case that is an x-reg ldp/stp, and the FPR case is a q-register load/store. As Richard pointed out in the PR, aarch64_get_separate_components already checks that the offsets are suitable for a single load, so we just need to choose a mode in aarch64_reg_save_mode that gives the full q-register range. In this patch, we choose V16QImode as an alternative 16-byte "bag-of-bits" mode that doesn't have the artificial range restrictions imposed on T{I,F,D}mode. For T{F,D}mode in GCC 15 I think we could consider relaxing the restriction imposed in aarch64_classify_address, as AFAIK T{F,D}mode can only be allocated to FPRs (unlike TImode). But such a change seems too invasive to consider for GCC 14 at this stage (let alone backports). Unlike for GCC 14 we need additional handling in the load/store pair code as various cases are not expecting to see V16QImode (particularly the writeback patterns, but also aarch64_gen_load_pair). gcc/ChangeLog: PR target/111677 * config/aarch64/aarch64.cc (aarch64_reg_save_mode): Use V16QImode for the full 16-byte FPR saves in the vector PCS case. (aarch64_gen_storewb_pair): Handle V16QImode. (aarch64_gen_loadwb_pair): Likewise. (aarch64_gen_load_pair): Likewise. * config/aarch64/aarch64.md (loadwb_pair<TX:mode>_<P:mode>): Rename to ... (loadwb_pair<TX_V16QI:mode>_<P:mode>): ... this, extending to V16QImode. (storewb_pair<TX:mode>_<P:mode>): Rename to ... (storewb_pair<TX_V16QI:mode>_<P:mode>): ... this, extending to V16QImode. * config/aarch64/iterators.md (TX_V16QI): New. gcc/testsuite/ChangeLog: PR target/111677 * gcc.target/aarch64/torture/pr111677.c: New test.