From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com [IPv6:2a00:1450:4864:20::22d]) by sourceware.org (Postfix) with ESMTPS id 64C073858C01 for ; Tue, 17 Oct 2023 08:46:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 64C073858C01 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 64C073858C01 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::22d ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1697532415; cv=none; b=pEBQmIPjCNtmkNtWkTNtWl42RZJRHdrEQpVZqaMKrmsDMHI7reKj/pxxLbq8yLKXBIvh80mzxcY/9Y8f9JPucXP5DUP3v7onDmFADOZ7pRfKc+x5aE0nTRKaCNvKVViu9X8oLSAzkucl1Zc49ehTzQhyeKDClQfzGZcpRemJEF8= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1697532415; c=relaxed/simple; bh=2okhs0hho6/J/Z7ymNcSGQUQkalWemHhZDFhuFoVRAU=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=sk74llpFN9lSngYEXV8Ql51vYkR/scaiyIQgo2cNnCBBP3IY831zEv3UyVAct6rd5iUNDk5wKHQ2JJwwxUWEeeCRQC5x8UGydJr7FiGMM7TLCPGbOLsMvETZOhMbvBZZm6j1bPZ09GYqTzt2yjQEzN3mahJRtGqnKj8NP+gb/jM= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-lj1-x22d.google.com with SMTP id 38308e7fff4ca-2c51682fddeso30368511fa.1 for ; Tue, 17 Oct 2023 01:46:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1697532411; x=1698137211; darn=gcc.gnu.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=PGCYdxApQZGLzGarHIpGZkDQ8+09x3iN92XdYbLFo+0=; b=TDxQUP7p1eb2oldeJcyfo9fPmGdXaIPabLlqa+S298S0Dxp0yTQOri3pqa0AzTYw2U vp5YCsPd2dBb2gFOQuCR55QLcvmiGqR7U8zS8qUKHWBRzw6UzFdcT4tlk89tt9ZNV/XG mSp5T/bt2T37IETFmaFE5aFgMhL91J1Q85jCu08kZdChMWPh4B9GJtwumBaKRKIcXwSR o13MKMyNfDw+QxfcAW4KaTEbiKHDLqg0DKhP7XHYSIWy1WbBHBgK7GpOUolr5by5pdwg EzZsezoCR++Mcx/VuaKgKQud0DClgE2gHoNsH7s2ZK0IHexx85/9aUoREivSrvdM9LVZ NfEA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697532411; x=1698137211; h=content-transfer-encoding:cc: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=PGCYdxApQZGLzGarHIpGZkDQ8+09x3iN92XdYbLFo+0=; b=bkBA7ILBCw2+i9G8krCj+USxGiyzHv5ItbIyjuWFQHP/VClLLAofsqcKJTXEmiPBRh ik/EKIV6wWSGg2vJ7qc1/H3rrLsLbkZVAHqm8bqaB45eZ7wadLhTHydq5vagdNWy1VRS Ri1QSYc+fDnYkK3byF6pHlJ0JWI8B1scnaKbSLOuDY0n8+baB+xhsmf6waNzmP1WaQzB f4qYfR7lBrrsdpblZhnu67jvTdpOf5cCpSvn1pzJ1U++RuII/sq8OYxpIZNMz+hhAu4O BCvmsKFz6CrS4bfuCwOIOVmhjfH8oom0DR/tGUw/VtQ+rb3HbkmvLuPb9S2iuFhkAX72 VRNA== X-Gm-Message-State: AOJu0YxARmAKeJO/ISe7+e9puf18k3eTS465AvEgbkykSZd/w+GreS7R 3KQeM89LDJRWDH8TPzZ651XZLzWXNaymNvA61tY= X-Google-Smtp-Source: AGHT+IHBXjqxk+9wXaXPAH+tHE2Ar6xI/eQlPBXy5hIxsdTodmhosOpBdrYuywe+dX9BEA8tkuYWq10OqofU6PLr2i0= X-Received: by 2002:a2e:7204:0:b0:2c5:985:2484 with SMTP id n4-20020a2e7204000000b002c509852484mr610178ljc.4.1697532410653; Tue, 17 Oct 2023 01:46:50 -0700 (PDT) MIME-Version: 1.0 References: <01f701d9feeb$ce8654e0$6b92fea0$@nextmovesoftware.com> <443998e0-99f3-4b67-972e-0f6746cc4c9c@gmail.com> <007b01d9ff4c$ea0231b0$be069510$@nextmovesoftware.com> In-Reply-To: From: Richard Biener Date: Tue, 17 Oct 2023 10:43:53 +0200 Message-ID: Subject: Re: [PATCH] PR 91865: Avoid ZERO_EXTEND of ZERO_EXTEND in make_compound_operation. To: Jeff Law , Segher Boessenkool Cc: Roger Sayle , gcc-patches@gcc.gnu.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-1.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP 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 Mon, Oct 16, 2023 at 9:27=E2=80=AFPM Jeff Law wr= ote: > > > > On 10/15/23 03:49, Roger Sayle wrote: > > > > Hi Jeff, > > Thanks for the speedy review(s). > > > >> From: Jeff Law > >> Sent: 15 October 2023 00:03 > >> To: Roger Sayle ; gcc-patches@gcc.gnu.org > >> Subject: Re: [PATCH] PR 91865: Avoid ZERO_EXTEND of ZERO_EXTEND in > >> make_compound_operation. > >> > >> On 10/14/23 16:14, Roger Sayle wrote: > >>> > >>> This patch is my proposed solution to PR rtl-optimization/91865. > >>> Normally RTX simplification canonicalizes a ZERO_EXTEND of a > >>> ZERO_EXTEND to a single ZERO_EXTEND, but as shown in this PR it is > >>> possible for combine's make_compound_operation to unintentionally > >>> generate a non-canonical ZERO_EXTEND of a ZERO_EXTEND, which is > >>> unlikely to be matched by the backend. > >>> > >>> For the new test case: > >>> > >>> const int table[2] =3D {1, 2}; > >>> int foo (char i) { return table[i]; } > >>> > >>> compiling with -O2 -mlarge on msp430 we currently see: > >>> > >>> Trying 2 -> 7: > >>> 2: r25:HI=3Dzero_extend(R12:QI) > >>> REG_DEAD R12:QI > >>> 7: r28:PSI=3Dsign_extend(r25:HI)#0 > >>> REG_DEAD r25:HI > >>> Failed to match this instruction: > >>> (set (reg:PSI 28 [ iD.1772 ]) > >>> (zero_extend:PSI (zero_extend:HI (reg:QI 12 R12 [ iD.1772 ])))) > >>> > >>> which results in the following code: > >>> > >>> foo: AND #0xff, R12 > >>> RLAM.A #4, R12 { RRAM.A #4, R12 > >>> RLAM.A #1, R12 > >>> MOVX.W table(R12), R12 > >>> RETA > >>> > >>> With this patch, we now see: > >>> > >>> Trying 2 -> 7: > >>> 2: r25:HI=3Dzero_extend(R12:QI) > >>> REG_DEAD R12:QI > >>> 7: r28:PSI=3Dsign_extend(r25:HI)#0 > >>> REG_DEAD r25:HI > >>> Successfully matched this instruction: > >>> (set (reg:PSI 28 [ iD.1772 ]) > >>> (zero_extend:PSI (reg:QI 12 R12 [ iD.1772 ]))) allowing > >>> combination of insns 2 and 7 original costs 4 + 8 =3D 12 replacement > >>> cost 8 > >>> > >>> foo: MOV.B R12, R12 > >>> RLAM.A #1, R12 > >>> MOVX.W table(R12), R12 > >>> RETA > >>> > >>> > >>> This patch has been tested on x86_64-pc-linux-gnu with make bootstrap > >>> and make -k check, both with and without --target_board=3Dunix{-m32} > >>> with no new failures. Ok for mainline? > >>> > >>> 2023-10-14 Roger Sayle > >>> > >>> gcc/ChangeLog > >>> PR rtl-optimization/91865 > >>> * combine.cc (make_compound_operation): Avoid creating a > >>> ZERO_EXTEND of a ZERO_EXTEND. > >>> > >>> gcc/testsuite/ChangeLog > >>> PR rtl-optimization/91865 > >>> * gcc.target/msp430/pr91865.c: New test case. > >> Neither an ACK or NAK at this point. > >> > >> The bug report includes a patch from Segher which purports to fix this= in simplify- > >> rtx. Any thoughts on Segher's approach and whether or not it should b= e > >> considered? > >> > >> The BZ also indicates that removal of 2 patterns from msp430.md would = solve this > >> too (though it may cause regressions elsewhere?). Any thoughts on tha= t approach > >> as well? > >> > > > > Great questions. I believe Segher's proposed patch (in comment #4) was= an > > msp430-specific proof-of-concept workaround rather than intended to be = fix. > > Eliminating a ZERO_EXTEND simply by changing the mode of a hard registe= r > > is not a solution that'll work on many platforms (and therefore not rea= lly suitable > > for target-independent middle-end code in the RTL optimizers). > Thanks. I didn't really look at Segher's patch, so thanks for digging > into it. Certainly just flipping the mode of the hard register isn't > correct. > > > > > > The underlying issue, which is applicable to all targets, is that combi= ne.cc's > > make_compound_operation is expected to reverse the local transformation= s > > made by expand_compound_operation. Hence, if an RTL expression is > > canonical going into expand_compound_operation, it is expected (hoped) > > to be canonical (and equivalent) coming out of make_compound_operation. > In theory, correct. > > > > > > Hence, rather than be a MSP430 specific issue, no target should expect = (or > > be expected to see) a ZERO_EXTEND of a ZERO_EXTEND, or a SIGN_EXTEND > > of a ZERO_EXTEND in the RTL stream. Much like a binary operator with t= wo > > CONST_INT operands, or a shift by zero, it's something the middle-end m= ight > > reasonably be expected to clean-up. [Yeah, I know... =F0=9F=98=8A] > Agreed. > > > > > > >>> (set (reg:PSI 28 [ iD.1772 ]) > >>> (zero_extend:PSI (zero_extend:HI (reg:QI 12 R12 [ iD.1772 ])))) > > > > As a rule of thumb, if the missed optimization bug report includes comb= ine's > > diagnostic "Failed to match this instruction:", things can be improved = by adding > > a pattern (often a define_insn_and_split) that matches the shown RTL. > Yes, but we also need to ponder if that's the right way to fix any given > problem. Sometimes we're going to be better off simplifying elsewhere > in the pipeline. I think we can agree this is one of the cases where > matching the RTL in the backend is not the desired approach. > > Occasionally things like those two patterns show up for various reasons. > Hopefully they can be removed :-) Though the first looks awful close > to something I did for the mn102 (not to be confused with the mn103) > eons ago. Partial modes aren't exactly handled well. > > > > > In this case the perhaps reasonable assumption is that the above would/= should > > (normally) match the backend's existing (zero_extend:PSI (reg:QI ...)) = insn pattern. > > Or that's my understanding of why this PR is classified as rtl-optimiza= tion and > > not target. > I wouldn't put a lot of faith in the classification ;-) > > > > > > Finally, my patch shouldn't block a (updated corrected) variant of Segh= er's patch > > or other solution to PR 91865. The more (safe) solutions the merrier. > Generally agreed. Looking at the patch I wonder whether handling (zero_extend (zero_extend ..= )) shouldn't be done by using simplify_unary_operation instead of simplify_const_unary_operation here? If that's by design then I agree the patch looks reasonable (albeit = ugly) as long as the reverse still works. But you probably need Seghers ack here. Thanks, Richard. > jeff