From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 49D533858D20 for ; Wed, 5 Apr 2023 16:48:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 49D533858D20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1680713310; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:in-reply-to:in-reply-to: references:references; bh=uEWs6uhTKwK6yf6rWbCOBG2EytIlvwfGBJIwEy1lL2c=; b=JkXAFeU0k/aWBkj3qJnfA+oZAIV3OeDBWbjfwxuNZEUdJhNh/BXNhnEIH/nHxIN6l63jNM C+0CZbLDNdZ1buMQNZOoRuuz7ZOP8O2aAJOCfLrJRslyCEaZxL24IYIkcVZPtUGfdphTj7 cs0LiT1FvNBaENz+n5rvoDPGR3R43t0= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-642-Iyd5NAb4NICF8xdI23LrNw-1; Wed, 05 Apr 2023 12:48:29 -0400 X-MC-Unique: Iyd5NAb4NICF8xdI23LrNw-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 2BD6238221C8; Wed, 5 Apr 2023 16:48:29 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.16]) by smtp.corp.redhat.com (Postfix) with ESMTPS id C993A2166B26; Wed, 5 Apr 2023 16:48:28 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 335GmP7Y3977447 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Wed, 5 Apr 2023 18:48:26 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 335GmO9Q3977446; Wed, 5 Apr 2023 18:48:24 +0200 Date: Wed, 5 Apr 2023 18:48:23 +0200 From: Jakub Jelinek To: Jeff Law Cc: Richard Biener , Richard Sandiford , Eric Botcazou , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] dse: Handle SUBREGs of word REGs differently for WORD_REGISTER_OPERATIONS targets [PR109040] Message-ID: Reply-To: Jakub Jelinek References: <8e0e3cd5-e4db-ce8a-b7dc-baac32aed516@gmail.com> MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.6 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-3.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,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 Wed, Apr 05, 2023 at 10:17:59AM -0600, Jeff Law wrote: > > It is true that an instruction like > > (insn 8 7 9 2 (set (reg:HI 141) > > (subreg:HI (reg:SI 142) 0)) "aauu.c":6:18 181 {*movhi_internal} > > (nil)) > > can appear in the IL on WORD_REGISTER_OPERATIONS target, but I think the > > upper bits shouldn't be random garbage in that case, it should be zero > > extended or sign extended. > Well, that's one of the core questions here. What are the state of the > upper 16 bits of (reg:HI 141)? The WORD_REGISTER_OPERATIONS docs aren't > 100% clear as we're not really doing any operation. > > So again, I think we need to decide if the DSE transformation is correct or > not. I *think* we can aggree that insn 39 is OK. It's really the semantics > of insn 47 that I think we need to agree on. What is the state of the upper > 16 bits of (reg:HI 175) after insn 47? I'm afraid I don't know the answers here, I think Eric is WORD_REGISTER_OPERATIONS expert here I think these days (most of the major targets are !WORD_REGISTER_OPERATIONS). Intuitively, WORD_REGISTER_OPERATIONS from the description would be that (insn 47 35 39 2 (set (reg:HI 175) (subreg:HI (reg:SI 166) 0)) "pr109040.c":9:11 179 {*movhi_internal} (expr_list:REG_DEAD (reg:SI 166) (nil))) would copy not just the low 16-bits of pseudo 166 to 175, but also the upper 16-bits. But if that is so, then something is broken in the code below. Some archeology shows that we were doing that on all arches initially and then Wed May 13 17:38:35 1998 Richard Kenner * combine.c (simplify_comparison, case AND): Don't commute AND with SUBREG if constant is whole mode and don't do if lowpart and not WORD_REGISTER_OPERATIONS. restricted that to WORD_REGISTER_OPERATIONS only. The whole optimization is then likely Wed Mar 18 05:54:25 1998 Richard Kenner * combine.c (gen_binary): Don't make AND that does nothing. (simplify_comparison, case AND): Commute AND and SUBREG. * i386.h (CONST_CONSTS, case CONST_INT): One-byte integers are cost 0. but the code has been tweaked further many times since then. > > We substitute that > > (leu:SI (and:SI (subreg:SI (reg:HI 175) 0) > > (const_int 2124 [0x84c])) > > (const_int 5 [0x5])) > > but then trigger the WORD_REGISTER_OPERATIONS block in simplify_comparison: > > /* If this is (and:M1 (subreg:M1 X:M2 0) (const_int C1)) where C1 > > fits in both M1 and M2 and the SUBREG is either paradoxical > > or represents the low part, permute the SUBREG and the AND > > and try again. */ > > if (GET_CODE (XEXP (op0, 0)) == SUBREG > > && CONST_INT_P (XEXP (op0, 1))) > > { > > unsigned HOST_WIDE_INT c1 = INTVAL (XEXP (op0, 1)); > > /* Require an integral mode, to avoid creating something like > > (AND:SF ...). */ > > if ((is_a > > (GET_MODE (SUBREG_REG (XEXP (op0, 0))), &tmode)) > > /* It is unsafe to commute the AND into the SUBREG if the > > SUBREG is paradoxical and WORD_REGISTER_OPERATIONS is > > not defined. As originally written the upper bits > > have a defined value due to the AND operation. > > However, if we commute the AND inside the SUBREG then > > they no longer have defined values and the meaning of > > the code has been changed. > > Also C1 should not change value in the smaller mode, > > see PR67028 (a positive C1 can become negative in the > > smaller mode, so that the AND does no longer mask the > > upper bits). */ > > && ((WORD_REGISTER_OPERATIONS > > && mode_width > GET_MODE_PRECISION (tmode) > > && mode_width <= BITS_PER_WORD > > && trunc_int_for_mode (c1, tmode) == (HOST_WIDE_INT) c1) > > || (mode_width <= GET_MODE_PRECISION (tmode) > > && subreg_lowpart_p (XEXP (op0, 0)))) > > && mode_width <= HOST_BITS_PER_WIDE_INT > > && HWI_COMPUTABLE_MODE_P (tmode) > > && (c1 & ~mask) == 0 > > && (c1 & ~GET_MODE_MASK (tmode)) == 0 > > && c1 != mask > > && c1 != GET_MODE_MASK (tmode)) > > { > > op0 = simplify_gen_binary (AND, tmode, > > SUBREG_REG (XEXP (op0, 0)), > > gen_int_mode (c1, tmode)); > > op0 = gen_lowpart (mode, op0); > > continue; > > } > > } > > c1 is 0x84c. I believe this is the exact spot where things go wrong, > > and is because for WORD_REGISTER_OPERATIONS we assume something that the > > DSE added instruction didn't guarantee. > pan2.li zero'd in on the same block of code. > > The leap I'm strugging with is the assertion that this combine code assumes > something that the DSE added instruction does not guarantee. That's why I > asked if we agreed that the before/after from DSE was correct or not. I > think that's still the outstanding question. Jakub