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 3A6AF3857808 for ; Tue, 5 Jan 2021 18:19:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 3A6AF3857808 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-433-9Kf7TQm0Pp-MNlD7QuNv6w-1; Tue, 05 Jan 2021 13:19:57 -0500 X-MC-Unique: 9Kf7TQm0Pp-MNlD7QuNv6w-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id E4F439CC03; Tue, 5 Jan 2021 18:19:55 +0000 (UTC) Received: from localhost.localdomain (ovpn-114-95.phx2.redhat.com [10.3.114.95]) by smtp.corp.redhat.com (Postfix) with ESMTP id 75BA56F812; Tue, 5 Jan 2021 18:19:55 +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> From: Jeff Law Message-ID: <23a47ef1-8782-e801-f604-8cd551abc784@redhat.com> Date: Tue, 5 Jan 2021 11:19:54 -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: <83c1fed5-d9aa-5f19-b04c-0ca432ffe183@linux.ibm.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 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=-6.0 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: Tue, 05 Jan 2021 18:20:00 -0000 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. OK for the trunk with an expanded comment. Thanks, jeff