From: Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org>
To: Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org>,
Richard Biener <rguenther@suse.de>,
gcc Patches <gcc-patches@gcc.gnu.org>,
richard.sandiford@arm.com
Subject: Re: [aarch64] Use dup and zip1 for interleaving elements in initializing vector
Date: Fri, 21 Apr 2023 20:45:41 +0530 [thread overview]
Message-ID: <CAAgBjMnaQYbotgGaSx_uJ6mUynF3be2jqD2e0OP90MziaXjUsA@mail.gmail.com> (raw)
In-Reply-To: <mptv8hpa92i.fsf@arm.com>
On Fri, 21 Apr 2023 at 14:47, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> > Hi,
> > I tested the interleave+zip1 for vector init patch and it segfaulted
> > during bootstrap while trying to build
> > libgfortran/generated/matmul_i2.c.
> > Rebuilding with --enable-checking=rtl showed out of bounds access in
> > aarch64_unzip_vector_init in following hunk:
> >
> > + rtvec vec = rtvec_alloc (n / 2);
> > + for (int i = 0; i < n; i++)
> > + RTVEC_ELT (vec, i) = (even_p) ? XVECEXP (vals, 0, 2 * i)
> > + : XVECEXP (vals, 0, 2 * i + 1);
> >
> > which is incorrect since it allocates n/2 but iterates and stores upto n.
> > The attached patch fixes the issue, which passed bootstrap, however
> > resulted in following fallout during testsuite run:
> >
> > 1] sve/acle/general/dupq_[1-4].c tests fail.
> > For the following test:
> > int32x4_t f(int32_t x)
> > {
> > return (int32x4_t) { x, 1, 2, 3 };
> > }
> >
> > Code-gen without patch:
> > f:
> > adrp x1, .LC0
> > ldr q0, [x1, #:lo12:.LC0]
> > ins v0.s[0], w0
> > ret
> >
> > Code-gen with patch:
> > f:
> > movi v0.2s, 0x2
> > adrp x1, .LC0
> > ldr d1, [x1, #:lo12:.LC0]
> > ins v0.s[0], w0
> > zip1 v0.4s, v0.4s, v1.4s
> > ret
> >
> > It shows, fallback_seq_cost = 20, seq_total_cost = 16
> > where seq_total_cost determines the cost for interleave+zip1 sequence
> > and fallback_seq_cost is the cost for fallback sequence.
> > Altho it shows lesser cost, I am not sure if the interleave+zip1
> > sequence is better in this case ?
>
> Debugging the patch, it looks like this is because the fallback sequence
> contains a redundant pseudo-to-pseudo move, which is costed as 1
> instruction (4 units). The RTL equivalent of the:
>
> movi v0.2s, 0x2
> ins v0.s[0], w0
>
> has a similar redundant move, but the cost of that move is subsumed by
> the cost of the other arm (the load from LC0), which is costed as 3
> instructions (12 units). So we have 12 + 4 for the parallel version
> (correct) but 12 + 4 + 4 for the serial version (one instruction too
> many).
>
> The reason we have redundant moves is that the expansion code uses
> copy_to_mode_reg to force a value into a register. This creates a
> new pseudo even if the original value was already a register.
> Using force_reg removes the moves and makes the test pass.
>
> So I think the first step is to use force_reg instead of
> copy_to_mode_reg in aarch64_simd_dup_constant and
> aarch64_expand_vector_init (as a preparatory patch).
Thanks for the clarification!
>
> > 2] sve/acle/general/dupq_[5-6].c tests fail:
> > int32x4_t f(int32_t x0, int32_t x1, int32_t x2, int32_t x3)
> > {
> > return (int32x4_t) { x0, x1, x2, x3 };
> > }
> >
> > code-gen without patch:
> > f:
> > fmov s0, w0
> > ins v0.s[1], w1
> > ins v0.s[2], w2
> > ins v0.s[3], w3
> > ret
> >
> > code-gen with patch:
> > f:
> > fmov s0, w0
> > fmov s1, w1
> > ins v0.s[1], w2
> > ins v1.s[1], w3
> > zip1 v0.4s, v0.4s, v1.4s
> > ret
> >
> > It shows fallback_seq_cost = 28, seq_total_cost = 16
>
> The zip verson still wins after the fix above, but by a lesser amount.
> It seems like a borderline case.
>
> >
> > 3] aarch64/ldp_stp_16.c's cons2_8_float test fails.
> > Test case:
> > void cons2_8_float(float *x, float val0, float val1)
> > {
> > #pragma GCC unroll(8)
> > for (int i = 0; i < 8 * 2; i += 2) {
> > x[i + 0] = val0;
> > x[i + 1] = val1;
> > }
> > }
> >
> > which is lowered to:
> > void cons2_8_float (float * x, float val0, float val1)
> > {
> > vector(4) float _86;
> >
> > <bb 2> [local count: 119292720]:
> > _86 = {val0_11(D), val1_13(D), val0_11(D), val1_13(D)};
> > MEM <vector(4) float> [(float *)x_10(D)] = _86;
> > MEM <vector(4) float> [(float *)x_10(D) + 16B] = _86;
> > MEM <vector(4) float> [(float *)x_10(D) + 32B] = _86;
> > MEM <vector(4) float> [(float *)x_10(D) + 48B] = _86;
> > return;
> > }
> >
> > code-gen without patch:
> > cons2_8_float:
> > dup v0.4s, v0.s[0]
> > ins v0.s[1], v1.s[0]
> > ins v0.s[3], v1.s[0]
> > stp q0, q0, [x0]
> > stp q0, q0, [x0, 32]
> > ret
> >
> > code-gen with patch:
> > cons2_8_float:
> > dup v1.2s, v1.s[0]
> > dup v0.2s, v0.s[0]
> > zip1 v0.4s, v0.4s, v1.4s
> > stp q0, q0, [x0]
> > stp q0, q0, [x0, 32]
> > ret
> >
> > It shows fallback_seq_cost = 28, seq_total_cost = 16
> >
> > I think the test fails because it doesn't match:
> > ** dup v([0-9]+)\.4s, .*
> >
> > Shall it be OK to amend the test assuming code-gen with patch is better ?
>
> Yeah, the new code seems like an improvement.
>
> > 4] aarch64/pr109072_1.c s32x4_3 test fails:
> > For the following test:
> > int32x4_t s32x4_3 (int32_t x, int32_t y)
> > {
> > int32_t arr[] = { x, y, y, y };
> > return vld1q_s32 (arr);
> > }
> >
> > code-gen without patch:
> > s32x4_3:
> > dup v0.4s, w1
> > ins v0.s[0], w0
> > ret
> >
> > code-gen with patch:
> > s32x4_3:
> > fmov s1, w1
> > fmov s0, w0
> > ins v0.s[1], v1.s[0]
> > dup v1.2s, v1.s[0]
> > zip1 v0.4s, v0.4s, v1.4s
> > ret
> >
> > It shows fallback_seq_cost = 20, seq_total_cost = 16
> > I am not sure how interleave+zip1 cost is lesser than fallback seq
> > cost for this case.
> > I assume that the fallback sequence is better here ?
>
> The fix for 1] works for this case too.
Indeed, I verified using force_reg fixes the issues.
I will send a follow up patch after the preparatory patch using force_reg.
Thanks,
Prathamesh
>
> Thanks,
> Richard
next prev parent reply other threads:[~2023-04-21 15:16 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-29 14:39 Prathamesh Kulkarni
2022-11-29 15:13 ` Andrew Pinski
2022-11-29 17:06 ` Prathamesh Kulkarni
2022-12-05 10:52 ` Richard Sandiford
2022-12-05 11:20 ` Richard Sandiford
2022-12-06 1:31 ` Prathamesh Kulkarni
2022-12-26 4:22 ` Prathamesh Kulkarni
2023-01-12 15:51 ` Richard Sandiford
2023-02-01 9:36 ` Prathamesh Kulkarni
2023-02-01 16:26 ` Richard Sandiford
2023-02-02 14:51 ` Prathamesh Kulkarni
2023-02-02 15:20 ` Richard Sandiford
2023-02-03 1:40 ` Prathamesh Kulkarni
2023-02-03 3:02 ` Prathamesh Kulkarni
2023-02-03 15:17 ` Richard Sandiford
2023-02-04 6:49 ` Prathamesh Kulkarni
2023-02-06 12:13 ` Richard Sandiford
2023-02-11 9:12 ` Prathamesh Kulkarni
2023-03-10 18:08 ` Richard Sandiford
2023-03-13 7:33 ` Richard Biener
2023-04-03 16:33 ` Prathamesh Kulkarni
2023-04-04 18:05 ` Richard Sandiford
2023-04-06 10:26 ` Prathamesh Kulkarni
2023-04-06 10:34 ` Richard Sandiford
2023-04-06 11:21 ` Prathamesh Kulkarni
2023-04-12 8:59 ` Richard Sandiford
2023-04-21 7:27 ` Prathamesh Kulkarni
2023-04-21 9:17 ` Richard Sandiford
2023-04-21 15:15 ` Prathamesh Kulkarni [this message]
2023-04-23 1:53 ` Prathamesh Kulkarni
2023-04-24 9:29 ` Richard Sandiford
2023-05-04 11:47 ` Prathamesh Kulkarni
2023-05-11 19:07 ` Richard Sandiford
2023-05-13 9:10 ` Prathamesh Kulkarni
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAAgBjMnaQYbotgGaSx_uJ6mUynF3be2jqD2e0OP90MziaXjUsA@mail.gmail.com \
--to=prathamesh.kulkarni@linaro.org \
--cc=gcc-patches@gcc.gnu.org \
--cc=rguenther@suse.de \
--cc=richard.sandiford@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).