From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x42f.google.com (mail-pf1-x42f.google.com [IPv6:2607:f8b0:4864:20::42f]) by sourceware.org (Postfix) with ESMTPS id 327193858C2C; Tue, 2 Jan 2024 16:55:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 327193858C2C 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 327193858C2C Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::42f ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1704214524; cv=none; b=LhdZ7AK/fJNL5kJY7pHBza+f1LE0z/zML0K3vtGXPDkiwQv0wiNKKTVhI9CphTPRL+8OXpDoeuRKCaHBDYZkB8bQRt4daHe+cDrVj/72m4jRiFZiLHiips1TXP9M7tHZxzYxdXywxadbH0e+0T0ig3mgw/Brd68TlJ6aZBseK3A= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1704214524; c=relaxed/simple; bh=7hlvHoGFi/wUdm3WrQLMP4IrhuWlitbD8vZAK7alJik=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=e54ZMQwTgEX0hDa1LI7X/+d85GdYCRpQ6HFCJdTcuEdLgUzuzmHnSfMlX0wKKd3QiYD/noTjbktsM42qAXb2p14QpQHlxLIWMdzFBmwj06OsOVbsi1ztc3ZCDI5CT8yFigSMGJj9qou2QV62JpA2Yv4JoGGcalvCLXKKTLl8yHM= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pf1-x42f.google.com with SMTP id d2e1a72fcca58-6d9af1f52bcso2290393b3a.3; Tue, 02 Jan 2024 08:55:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1704214518; x=1704819318; darn=gcc.gnu.org; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=74gUX34SQj9jNhn59DLUMrunevnGGAGOyZ6Aqd79JmE=; b=QnnUDoDqWvO1yn4/xdQXDPY+E0g81qNOLRbcc8BV05lbwWG47d2xMk+CbXDnMsqfv4 icArnFaB1y2V8HmucfWMFY6bZuNsB6mXvZQ/3ODzXUpC0spreCdlRItA5T4RplqT4f3i WXwKcueRP/sU8gtx9tslIjBcEfdTiTWQ3ypnIy8oivM3PhsIY2wFJW82zfgN/u1qZ2Ir b3De3PGPBEc1fkh9dP+ucMgje+dawBeDfdJ733/WyWJNjskJ2IMbdkaLjxqDF/OKCmPb dGzabngZIK2im2Du88ydkalwUslAqDVBo05iuEmnf0/FKkF4XfYwILuiExXaBM49p8B7 3+IA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1704214518; x=1704819318; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=74gUX34SQj9jNhn59DLUMrunevnGGAGOyZ6Aqd79JmE=; b=p9nw+ydcOf88RSjLXAqtqLKQv1+A77kw6Mzz9K/C3/2jmuqfbbO9rARdPDRluFRsqx R3GL/5lsqS9lyINXroGgh5aX1NXgy1FSgBLOpE0FOPknwpr4IExMI9+FwDcA3PrFo7zh 0J14fLcuuIfJurDQw3TP55IL5W/sI4S0jzdvTwHXxrVsrRiH6TbNxEGcTrcN9yAJP2X7 /ISn76solFfd75EyPl/IPh1fUrrFwL/WruwmOuPjZT1ZBCUwtGzT4RRTNUDrEmernKvt 7WebMEuDXWaQamqeUxObvDB/vHVMnf9707etKvHxningmSNEXrCuvOx0GoBo60VbONR5 oiZQ== X-Gm-Message-State: AOJu0Yxu5WoyTvRt9AQUg8GRZJ3iabyQV5R90nDlAk1tWkhylTA/7Pxu s2ytbtILqZkhFQJSuOlmhxw= X-Google-Smtp-Source: AGHT+IHraNkgYKbHFAyTN6BmeAPNrIWeShCWqBh6/EsysnQqpZ7LCoRrnkC+SaXT4K/o26Po8ohCiQ== X-Received: by 2002:a05:6a00:4b8b:b0:6da:9928:c9b9 with SMTP id ks11-20020a056a004b8b00b006da9928c9b9mr1040298pfb.24.1704214517944; Tue, 02 Jan 2024 08:55:17 -0800 (PST) Received: from [172.31.0.109] ([136.36.72.243]) by smtp.gmail.com with ESMTPSA id y25-20020a62b519000000b006d9b98a8ea4sm14920977pfe.159.2024.01.02.08.55.16 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 02 Jan 2024 08:55:17 -0800 (PST) Message-ID: <388e329a-032e-40e6-baca-3147aa39e66c@gmail.com> Date: Tue, 2 Jan 2024 09:55:06 -0700 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] Improved RTL expansion of field assignments into promoted registers. Content-Language: en-US To: Roger Sayle , gcc-patches@gcc.gnu.org Cc: 'YunQiang Su' References: <005901da399e$7d13b330$773b1990$@nextmovesoftware.com> <002901da39c4$f714f510$e53edf30$@nextmovesoftware.com> From: Jeff Law In-Reply-To: <002901da39c4$f714f510$e53edf30$@nextmovesoftware.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-2.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,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 12/28/23 12:35, Roger Sayle wrote: > > Hi Jeff, > Thanks for the speedy review. > >> On 12/28/23 07:59, Roger Sayle wrote: >>> This patch fixes PR rtl-optmization/104914 by tweaking/improving the >>> way that fields are written into a pseudo register that needs to be >>> kept sign extended. >> Well, I think "fixes" is a bit of a stretch. We're avoiding the issue by changing the >> early RTL generation, but if I understand what's going on in the RTL optimizers >> and MIPS backend correctly, the core bug still remains. Admittedly I haven't put it >> under a debugger, but that MIPS definition of NOOP_TRUNCATION just seems >> badly wrong and is just waiting to pop it's ugly head up again. > > I think this really is the/a correct fix. The MIPS backend defines NOOP_TRUNCATION > to false, so it's not correct to use a SUBREG to convert from DImode to SImode. > The problem then is where in the compiler (middle-end or backend) is this invalid > SUBREG being created and how can it be fixed. In this particular case, the fault > is in RTL expansion. There may be other places where a SUBREG is inappropriately > used instead of a TRUNCATE, but this is the place where things go wrong for > PR rtl-optimization/104914. Maybe a better way to put it is I think you're patch one piece of the solution, but we still have the potential for bugs due to the seemingly bogus defintion of TRULY_NOOP_TRUNCATION in the mips port. > > Once an inappropriate SImode SUBREG is in the RTL stream, it can remain > harmlessly latent (most of the time), unless it gets split, simplified or spilled. > Copying this SImode expression into it's own pseudo, results in incorrect code. > One approach might be to use an UNSPEC for places where backend > invariants are temporarily invalid, but in this case it's machine independent > middle-end code that's using SUBREGs as though the target was an x86/pdp11. > > So I agree that on the surface, both of these appear to be identical: >> (set (reg:DI) (sign_extend:DI (truncate:SI (reg:DI)))) >> (set (reg:DI) (sign_extend:DI (subreg:SI (reg:DI)))) > > But should they get split or spilled by reload: Even if they're not spilled, on a target like mips they're not equivalent. It highlights the poor design around TRULY_NOOP_TRUNCATION. Essentially it's out of band information on how RTL need to be interpreted. We have similar problems with SHIFT_COUNT_TRUNCATED and other macros. >>> 2023-12-28 Roger Sayle >>> >>> gcc/ChangeLog >>> PR rtl-optimization/104914 >>> * expr.cc (expand_assignment): When target is >> SUBREG_PROMOTED_VAR_P >>> a sign or zero extension is only required if the modified field >>> overlaps the SUBREG's most significant bit. On MODE_REP_EXTENDED >>> targets, don't refer to the temporarily incorrectly extended value >>> using a SUBREG, but instead generate an explicit TRUNCATE rtx. >> [ ... ] >> >> >>> + /* Check if the field overlaps the MSB, requiring extension. */ >>> + else if (known_eq (bitpos + bitsize, >>> + GET_MODE_BITSIZE (GET_MODE (to_rtx)))) >> Do you need to look at the size of the field as well? ie, the starting position might >> be before the sign bit, but the width of the field might cover the mode's sign bit? >> >> I'm not real good in the RTL expansion code, so if I'm offbase on this, just let me >> know. > > There are two things that help here. The first is that the most significant > bit never appears in the middle of a field, so we don't have to worry about > overlapping, nor writes to the paradoxical bits of the SUBREG. And secondly, > bits are numbered from zero for least significant, to MODE_BITSIZE (mode) - 1 > for most significant, irrespective of the endian-ness. So the code only needs > to check the highest value bitpos + bitsize is the maximum value for the mode. > The above logic stays the same, but which byte insert requires extension will > change between mips64be and mips64le. i.e. we test that the most significant > bit of the field/byte being written in the most significant bit of the SUBREG > target. [That's my understanding/rationalization, I could wrong]. Yea, if the type is mode M, then we won't have a field that exceeds the size of M. So we just need to know if the position + size lands squarely on the MSB. Thanks for walking me what should have been fairly obvious in retrospect. > > One thing I could be more cautious about is using maybe_eq instead of > known_eq, but the rest of the code (including truly_noop_truncation) assumes > scalar integer modes, so variable length vectors aren't (yet) a concern. > Would using maybe_eq be better coding style? It's probably better to use maybe_eq for future proofing. OK with that change after the usual testing. Thanks diving into this. jeff