From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x632.google.com (mail-ej1-x632.google.com [IPv6:2a00:1450:4864:20::632]) by sourceware.org (Postfix) with ESMTPS id E4705384C002 for ; Thu, 9 Sep 2021 06:16:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E4705384C002 Received: by mail-ej1-x632.google.com with SMTP id me10so1271884ejb.11 for ; Wed, 08 Sep 2021 23:16:28 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=VZSuE3GfDGPU4XqH8a1Iysr5UkA/Asc392BxoqvQYhw=; b=LZor2e6hln9UHllKnAxwy1BtmlYJRb3Yqqkqsc1n/GYpRdQayLm+OEEhKbevKqPAB3 nLABgBtmfQQQawcMIlc0mR5L2z66Klbus1tAT0wgBY0K2SLlulBMe5i0VdlpLPJmWjrt agAl3mJor0QaFu+A85Zk/f8FeO9JT7g+eZUkvlsSACdPrrQ4BBS8D3G1fmL9io4l8ogf Lj8WjRgog5WFBy8XfHn9sOZFDHA2FG13u84gZjOMtO/gImOT3C7G6qVJrHxRGxrZkmhS fMdHcYGRE9aq98ouTj0WPcXyXVo3h2KDeZjwDjcHiYRnJPFLWTCvaeh0jV0f0jZwFsVb pzVA== X-Gm-Message-State: AOAM533k3VmIWAh9lCbKcKaMJpp0oqMV0Kso+NhAsyTEkInI7Op4KPvD kXB8CBVCbaSRpaLORmv/NAsj5wWTCbHMzf5XXkI= X-Google-Smtp-Source: ABdhPJyalBVk+iSrxxhnQySeFNjehFJbXvGC+9L8NRs7jIAhu2RQvrWEIc9k+Q6qWVqlWiKr3Z+m7PAfemD+Df85h5g= X-Received: by 2002:a17:906:194e:: with SMTP id b14mr1541349eje.571.1631168187903; Wed, 08 Sep 2021 23:16:27 -0700 (PDT) MIME-Version: 1.0 References: <20210907230730.GM1583@gate.crashing.org> <20210908170809.GP1583@gate.crashing.org> <5DBC1101-9DD6-48F8-BC25-F4DD354B4D74@gmail.com> <20210908191602.GQ1583@gate.crashing.org> In-Reply-To: <20210908191602.GQ1583@gate.crashing.org> From: Richard Biener Date: Thu, 9 Sep 2021 08:16:16 +0200 Message-ID: Subject: Re: [PATCH] Fix SFmode subreg of DImode and TImode To: Segher Boessenkool Cc: Michael Meissner , GCC Patches , David Edelsohn , Bill Schmidt , Peter Bergner , Will Schmidt Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-2.9 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.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Sep 2021 06:16:30 -0000 On Wed, Sep 8, 2021 at 9:18 PM Segher Boessenkool wrote: > > On Wed, Sep 08, 2021 at 08:39:31PM +0200, Richard Biener wrote: > > On September 8, 2021 7:08:09 PM GMT+02:00, Segher Boessenkool wrote: > > >It is not a good idea to do allow all those things. Most backends can > > >only support a few combinations of them, and everything else results in > > >*worse* machine code, best case, and more and more complicated (and more > > >buggy!) backend code. > > > > > >But that is a code quality issue. The current problem is that we have > > >at least PR102211 and PR102154 (as well as reports elsewhere of bugs on > > >other targets). Code that used before doesn't anymore, and we have no > > >clear way out, no recommendation how to fix this and a) keep the same > > >functionality without huge changes, and b) keep the same machine code > > >quality. > > > > > >I do not think something like that can be done. That is why I am asking > > >for the patch to be reverted until all of the groundwork for it has been > > >done. That includes making generic testcases that show how such subregs > > >behave, so that we can see in testresults what changes do to existing > > >targets. > > > > Heh, I understood your earlier reply that you supported the change in principle based on the fact that nested subregs are invalid. > > Ah. No. I meant to lament the fact that we use subregs for multiple > things, so that doing a bit_cast of a real subreg has to be expressed as > just one subreg, which is clearly sub-optimal for most backends. > > I say *has to* because a subreg of a subreg is not valid RTL; it has to > be written as just one subreg. Which makes thing more confusing and > confused than this already non-trivial thing has to be. > > > Now, I don't think that validate_subreg is supposed to be the decision maker on what a target allows. > > Right, some target hook or macro or whatnot should. > > > For subregs of hardregs we seem to have a good way of validating, but > > what do we have for subregs of pseudos? Is it the passes generating > > the new unsupported subregs that should do different things? Should > > validate_subreg use a target hook to allow those special casings we > > removed which all were necessary just for specific targets but > > appearantly did not do any harm for other targets? > > Currently, we can disallow things in predicates and/or the instruction > conditions. > > > Can you give advice as to how to address the needs of the HFmode subregs on x86 if not by adding another (generic) narrow exception in validate_subreg? > > If I knew what the problem was, perhaps? Is this explained in some mail > somewhere? > > > That said, I fail to see a good way forward after now two appearantly failed attempts. > > > > Btw, I'm fine reverting the patch but then what's the solution here? > > I think we should (longer term) get rid of the overloaded meanings and > uses of subregs. One fairly simple thing is to make a new rtx code > "bit_cast" (or is there a nice short more traditional name for it?) But subreg _is_ bit_cast. What is odd to me is that a "disallowed" subreg like (subreg:SF (reg:TI ..) 0) magically becomes valid (in terms of validate_subreg) if you rewrite it as (subreg:SF (subreg:SI (reg:TI ..) 0) 0). Of course that's nested and invalid but just push the inner subreg to a new pseudo and the thing becomes valid. That was my original idea for dealing with the issue described in the thread including https://gcc.gnu.org/pipermail/gcc-patches/2021-August/578437.html where the original issue is that extract_integral_bit_field creates a subreg that's not valid according to validate_subreg. So I thought well, extracting bitfields should be done on integer modes (thus pun everything to ints and back), but that had even more(?) fallout. > But that is not the core problem we had here. The behaviour of the > generic parts of the compiler was changed, without testing if that > works on other targets but x86. That is an understandable mistake, it > takes some experience to know where the morasses are. But this change > should have been accompanied by testcases exercising the changed code. > We would have clearly seen there are issues then, simply by watching > gcc-testresults@ (and/or maintainers are on top of the test results > anyway). Also, if there were testcases for this, we could have some > confidence that a change in this area is robust. Well, that only works if some maintainers that are familiar enough with all this chime in ;) It's stage1 so it's understandable that some people (like me ...) are tyring to help people making progress even if that involves trying to decipher 30 years of GCC history in this area (without much success in the end as we see) ;) Given validate_subreg's _intent_ is to disallow float subregs that change size exceptions like /* ??? This should not be here. Temporarily continue to allow word_mode subregs of anything. The most common offender is (subreg:SI (reg:DF)). Generally, backends are doing something sketchy but it'll take time to fix them all. */ if (omode == word_mode) ; /* ??? Similarly, e.g. with (subreg:DF (reg:TI)). Though store_bit_field is the culprit here, and not the backends. */ else if (known_ge (osize, regsize) && known_ge (isize, osize)) ; look arbitrary and odd. Richard. > > Segher > > > p.s. Very unrelated... Should we have __builtin_bit_cast for C as well? > Is there any reason this could not work?