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 [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 8F04C3870856 for ; Fri, 8 Jan 2021 20:37:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 8F04C3870856 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-555-e2dEFg8VMX6JQtz66YZHfg-1; Fri, 08 Jan 2021 15:37:09 -0500 X-MC-Unique: e2dEFg8VMX6JQtz66YZHfg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 2BBA8801B12; Fri, 8 Jan 2021 20:37:08 +0000 (UTC) Received: from localhost.localdomain (ovpn-114-217.phx2.redhat.com [10.3.114.217]) by smtp.corp.redhat.com (Postfix) with ESMTP id 976B05B6A7; Fri, 8 Jan 2021 20:37:07 +0000 (UTC) Subject: Re: [PATCH] ira: Skip some pseudos in move_unallocated_pseudos To: "Kewen.Lin" Cc: Bill Schmidt , GCC Patches , Segher Boessenkool References: <6e1b52f1-a038-9725-38af-5e3007023718@linux.ibm.com> <20201222135546.GD2672@gate.crashing.org> <07bd112b-a767-5ba6-720e-3e8873c72d42@linux.ibm.com> <710726bc-8e7e-4799-cd4b-72df1e427759@redhat.com> <83c1fed5-d9aa-5f19-b04c-0ca432ffe183@linux.ibm.com> <23a47ef1-8782-e801-f604-8cd551abc784@redhat.com> <61a54418-bfdc-acc4-9896-cea9e6e84520@linux.ibm.com> From: Jeff Law Message-ID: <2d15789d-0c1c-5761-35ea-a331c31fa5da@redhat.com> Date: Fri, 8 Jan 2021 13:37:07 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: <61a54418-bfdc-acc4-9896-cea9e6e84520@linux.ibm.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Content-Language: en-US X-Spam-Status: No, score=-5.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Fri, 08 Jan 2021 20:37:15 -0000 On 1/5/21 8:12 PM, Kewen.Lin wrote: > on 2021/1/6 上午2:19, Jeff Law wrote: >> >> On 1/4/21 7:36 PM, Kewen.Lin wrote: >>> Hi Jeff, >>> >>> on 2021/1/5 上午7:13, Jeff Law wrote: >>>> On 12/22/20 11:40 PM, Kewen.Lin via Gcc-patches wrote: >>>>> Hi Segher, >>>>> >>>>> on 2020/12/22 下午9:55, Segher Boessenkool wrote: >>>>>> Hi! >>>>>> >>>>>> Just a dumb formatting comment: >>>>>> >>>>>> On Tue, Dec 22, 2020 at 04:05:39PM +0800, Kewen.Lin wrote: >>>>>>> This patch is to make move_unallocated_pseudos consistent >>>>>>> to what we have in function find_moveable_pseudos, where we >>>>>>> record the original pseudo into pseudo_replaced_reg only if >>>>>>> validate_change succeeds with newreg. To ensure every >>>>>>> unallocated pseudo in move_unallocated_pseudos has expected >>>>>>> information, it's better to add a check and skip it if it's >>>>>>> unexpected. This avoids possible ICEs in future. >>>>>>> >>>>>>> btw, I happened to found this in the bootstrapping for one >>>>>>> experimental local patch, which is considered as impractical. >>>>>>> --- a/gcc/ira.c >>>>>>> +++ b/gcc/ira.c >>>>>>> @@ -5111,6 +5111,11 @@ move_unallocated_pseudos (void) >>>>>>> { >>>>>>> int idx = i - first_moveable_pseudo; >>>>>>> rtx other_reg = pseudo_replaced_reg[idx]; >>>>>>> + /* If there is no appropriate pseudo in pseudo_replaced_reg, it >>>>>>> + means validate_change fails for this new pseudo in function >>>>>>> + find_moveable_pseudos, then bypass it here.*/ >>>>>> Dot space space. >>>>> Good catch, thanks! I forgot to reformat after polishing the comments. >>>>> Will fix it with other potential comments. >>>>> >>>>>> The patch sounds fine to me. Hard to tell without seeing the patch that >>>>>> exposed the problem (for onlookers like me who do not know this code >>>>>> well, anyway ;-) ) >>>>> The patch which made this issue exposed looks like: >>>>> >>>>> +; Like *rotl3_insert_3 but work with nonzero_bits rather than >>>>> +; explicit AND. >>>>> +(define_insn "*rotl3_insert_8" >>>>> + [(set (match_operand:GPR 0 "gpc_reg_operand" "=r") >>>>> + (ior:GPR (ashift:GPR (match_operand:GPR 1 "gpc_reg_operand" "r") >>>>> + (match_operand:SI 2 "u6bit_cint_operand" "n")) >>>>> + (match_operand:GPR 3 "gpc_reg_operand" "0")))] >>>>> + "HOST_WIDE_INT_1U << INTVAL (operands[2]) >>>>> + > nonzero_bits (operands[3], mode)" >>>>> +{ >>>>> + if (mode == SImode) >>>>> + return "rlwimi %0,%1,%h2,0,31-%h2"; >>>>> + else >>>>> + return "rldimi %0,%1,%H2,0"; >>>>> +} >>>>> + [(set_attr "type" "insert")]) >>>>> >>>>> Some insn matches this pattern in combine, later ira tries to introduce >>>>> one new pseudo since it meets the checks in find_moveable_pseudos, but >>>>> it fails in the call to validate_change since the nonzero_bits is more >>>>> rough and can't satisfy the pattern condition, leaving the unexpected >>>>> entry in pseudo_replaced_reg. >>>> But what doesn't make any sense to me is pseudo_replaced_reg[] is only >>>> set when validation is successful in find_moveable_pseudos.   So I can't >>>> see how this patch actually helps the problem you're describing. >>>> >>> Yeah, pseudo_replaced_reg[] is only set when validation is successful, >>> but we bump the max pseudo number in ira_create_new_reg as below >>> regardless of whether validation succeeds or not: >>> >>> rtx newreg = ira_create_new_reg (def_reg); >>> if (validate_change (def_insn, DF_REF_REAL_LOC (def), newreg, 0)) >>> >>> Later in move_unallocated_pseudos, the iterating could cover those >>> pseudos which were created but not used due to failed validation. >>> >>> for (i = first_moveable_pseudo; i < last_moveable_pseudo; i++) >>> if (reg_renumber[i] < 0) >>> { >>> int idx = i - first_moveable_pseudo; >>> rtx other_reg = pseudo_replaced_reg[idx]; // (1) >>> rtx_insn *def_insn = DF_REF_INSN (DF_REG_DEF_CHAIN (i)); >>> /* The use must follow all definitions of OTHER_REG, so we can >>> insert the new definition immediately after any of them. */ >>> df_ref other_def = DF_REG_DEF_CHAIN (REGNO (other_reg)) >>> >>> Then we can get the NULL other_reg in (1), also have unexpected df info >>> which causes ICE. The patch skips the handlings on those pseudos which >>> were intended to be used in validatation INSN but failed to. >> I was wondering if it was somehow related to creation of new pseudos.  >> The other important tidbit here is we reset last_movable_pseudo near the >> end of find_moveable_pseudos. > Yeah, the iterating will scan all new pseudos created in find_moveable_pseudos, > the problem occurs on those ones that fail to validate. > >> OK for the trunk with an expanded comment. > Thanks! Does the attached new version look good to you? Yes.  Thanks. jeff