From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 4C38B3858D37 for ; Fri, 21 Apr 2023 09:17:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 4C38B3858D37 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 8D1EA1480; Fri, 21 Apr 2023 02:17:55 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.110.72]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 298173F5A1; Fri, 21 Apr 2023 02:17:11 -0700 (PDT) From: Richard Sandiford To: Prathamesh Kulkarni Mail-Followup-To: Prathamesh Kulkarni ,Richard Biener , gcc Patches , richard.sandiford@arm.com Cc: Richard Biener , gcc Patches Subject: Re: [aarch64] Use dup and zip1 for interleaving elements in initializing vector References: Date: Fri, 21 Apr 2023 10:17:09 +0100 In-Reply-To: (Prathamesh Kulkarni's message of "Fri, 21 Apr 2023 12:57:32 +0530") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-24.7 required=5.0 tests=BAYES_00,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Prathamesh Kulkarni 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). > 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; > > [local count: 119292720]: > _86 = {val0_11(D), val1_13(D), val0_11(D), val1_13(D)}; > MEM [(float *)x_10(D)] = _86; > MEM [(float *)x_10(D) + 16B] = _86; > MEM [(float *)x_10(D) + 32B] = _86; > MEM [(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. Thanks, Richard