From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x32f.google.com (mail-wm1-x32f.google.com [IPv6:2a00:1450:4864:20::32f]) by sourceware.org (Postfix) with ESMTPS id 6CA4E3858C83 for ; Fri, 21 Apr 2023 15:16:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6CA4E3858C83 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org Received: by mail-wm1-x32f.google.com with SMTP id 5b1f17b1804b1-3f09b9ac51dso53420365e9.0 for ; Fri, 21 Apr 2023 08:16:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1682090178; x=1684682178; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :from:to:cc:subject:date:message-id:reply-to; bh=UEFAYlwhOgA3blOcis7IsG7wDiznBLyMqFfViLUs0bE=; b=s8l/o6246coD/BPCrksIHW/FzUW6N5gTK25RISOVZ2OH0U9J5JI/tZdUUPvqLDLCyc s0UbqktZQP3c5mEZuqzOaCmVraEv9uq55rSvxJ/OUR4sd+XRUI2a5DPNRBciwKsYB3C/ sOowiI2pWK/nI79sZmA5mjsHzj+Hp5vuqf/pURrOn2fG52YsTE4tzuH/OrzqqF+c23Yu d+eiXLd3TF7eP2kVSFOaimJ2Rr/jeZjU/Pn/PT0/jAkmT+Jc8q6huhfRoyqSAOnQiwjZ PESZx5TihI3McbrsTpJs8v0b8Hj/MsxfcF2x9cfa9T19y/ZUSSRC2m+G0utKpzc4Cm9b RfWQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1682090178; x=1684682178; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=UEFAYlwhOgA3blOcis7IsG7wDiznBLyMqFfViLUs0bE=; b=Lu2UpVvhO/dF71S+jnJd1pQVrJ4ZT5Z4+TehqtZc0L/C2UtJrUAzXHPqXRguMjlXJ4 1nDFwewjh1zqW/peIIKzKhNfp4MO5u+m0J1yQljROlRlxlv9jS5DMyvYpvLHubPrYCgp WuY1AzW0pWNL2FPFibMd6iX9MqnT7NNJ2OzAE7EoDeZUUN4fQnB5T4Z57LcEpjR53ces kx8LjYWgvI3sBhtCpO/46ckQK9c7ZLXN46zOkT9Kw3ZoS49dx0QZNZxaQDERflcZMqBy Q3ZzcP2UG8lIcOZqPYgDLDnMzXa1y2axDbSLjyCbY1ltEX0WKaJgEU8r3mNxtqD4WeYK lE6Q== X-Gm-Message-State: AAQBX9cRPqRxLOc/KD8BQiejB74glazh683O4EyfmYE/gYotOiZKkeQP MFQEt4xR08bChsXQS5ZqtjSgpyXmXOQOxt7Aq8rRHg== X-Google-Smtp-Source: AKy350aG5keYd6iQASsGX0cB2vG4iP0q6oobMejqLtIdsMFHYFlv18K63fbw0wVIcabARybp4Vp7H4D2ivSpkSlSoBE= X-Received: by 2002:a5d:620b:0:b0:2fb:48a9:f554 with SMTP id y11-20020a5d620b000000b002fb48a9f554mr8483121wru.2.1682090177676; Fri, 21 Apr 2023 08:16:17 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Prathamesh Kulkarni Date: Fri, 21 Apr 2023 20:45:41 +0530 Message-ID: Subject: Re: [aarch64] Use dup and zip1 for interleaving elements in initializing vector To: Prathamesh Kulkarni , Richard Biener , gcc Patches , richard.sandiford@arm.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Fri, 21 Apr 2023 at 14:47, Richard Sandiford wrote: > > 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). 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; > > > > [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. 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