From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x136.google.com (mail-lf1-x136.google.com [IPv6:2a00:1450:4864:20::136]) by sourceware.org (Postfix) with ESMTPS id AF97C3858D33 for ; Thu, 23 Nov 2023 08:24:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org AF97C3858D33 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 AF97C3858D33 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::136 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1700727862; cv=none; b=Gc9Iafj0xH+USMR7zLfuGCKiDs1mGAJQ8us1ME5fwzCpbilAGNJInT/7CvQ1/jauf2sppaALCP1fM5a92d/nDkql1n8wz9m24E1wa9coAD4+b96bit6MKnuJNzIfiU/+425VGXg8R0DgZPw0+TEejJl43YTwfuEiBiGpJxwxzi0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1700727862; c=relaxed/simple; bh=hWSmS7R7IgV9AFO6eyBimV99jbLr/PDWBsArFuJTF8Q=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=WbcsJxWV3mF7DaFO5ZmGOKDOZTYZT87ad4ziFOrHB63pCspgITCQ115DP4txFy3eZ3uWMW0Cr0zDSq4hu5o7TMrG8cU8q5QA3+5iVqtv0+X8ZOS6DEb8DX7hDuVx4CWCUky1ISwMl+5pIgTeHXAc6jN0U0dP0GG9+HHZFRdvVpQ= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-lf1-x136.google.com with SMTP id 2adb3069b0e04-507975d34e8so801106e87.1 for ; Thu, 23 Nov 2023 00:24:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1700727852; x=1701332652; darn=gcc.gnu.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=qN5dGUfn1lQLUCQRjg/+uN2OBHjDFnJAa0Rr0SPva1g=; b=WMFSG7p3UkTdJBsPNsjnyFL8YjzmTQNhWgpLAOQ7Bjbw5KCg9vxSmO2kJUYwFsDFAr fa8VSBLZyzQQIKm5dEWZZCOjzlx3IEi1kWPkZlya5GOTXe6xSSwlbgp+U6IqByd8YzW3 MnDXBETfY77krKLUJw1XhLFWvtV/pmqLOx8gpfM789gNz1rZCgz321E59N53a8OL8CTN ZApKVWn3if9DKDyAPkEwA84ECZXdwqAPqC1N6o/uxNtQEnHlvvRUSfJt/8hr41j/cfM0 +z0BhCBYeASnDpihUwdEcpw0tJXPnr1Fd0eyeucBDW6u1IpIryiFgFtPTy4xrxIcwGJz vw/A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700727852; x=1701332652; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=qN5dGUfn1lQLUCQRjg/+uN2OBHjDFnJAa0Rr0SPva1g=; b=k7nj28UvlA+9BLcd/ZekS/DYpFDkEd3INtZ5YZWfwzieKNtqhwseet6mmZvwzk4uPw MAs6zee+yPzt5e52SR8fDOeOQ0CuYBfvDQKqHpyd6RJFhfbVhCXbPzbqHhb0WTAvpMab Q2qH+Amb+nRAsVmEI17i7+AjnhvRqEXRDWm0P37uiYJW9fMVYRoBhdQXJ4/0IRf9xadE ZwhJk78ruK8AM1//LFsV9yewbXkIBrOMkN9bEziXN78gTJwha6jzTDcdyXrO3lCdxDK2 44au20qclrUuNPUWotBMd8U1VsJjGqRHnPd/VpI6WUsRyG/2Wi3/6VzDHr/wE5Zbpq58 jvvA== X-Gm-Message-State: AOJu0YxKvcSgcZSE5U8Jkx+btPW3Y21eAFNWaKm9YtXG3ukx7/QDe3e0 Qk9/yX8BVzTnDQHS61KEJOCeQnFpmOYiTYzNHnQ= X-Google-Smtp-Source: AGHT+IFEJRO33Ycxrgx8U7SfA9EEUuKhsaK1Tc9QSnDL+wubHJnCUeiIeHVsNSFfv/fXQPqhfjmN6+99+SdESejNH18= X-Received: by 2002:a05:6512:304e:b0:507:b099:749d with SMTP id b14-20020a056512304e00b00507b099749dmr4831669lfb.15.1700727851587; Thu, 23 Nov 2023 00:24:11 -0800 (PST) MIME-Version: 1.0 References: <85b4098e-a72f-d013-ff17-8097971f71ba@linux.ibm.com> <09FEFDAE-698B-4B06-A896-8088B9B31539@linaro.org> <4675c26c-f230-b6d6-27c5-bc9f74736e38@linux.ibm.com> <41a4d065-c4b6-4a67-adf0-e84e942616c7@gmail.com> <93ce3468-a1ee-e77c-cbeb-a8c67a303bf9@ispras.ru> <7eb725d9-de7c-87ba-5ebd-f2e1485c5854@ispras.ru> <475af219-f250-a0f4-78b0-998f96fb24aa@linux.ibm.com> <9874e072-747a-39e9-da5e-d88f77b275aa@linux.ibm.com> <434b7d49-808e-5254-b023-a7e1dad29f81@ispras.ru> <1f407c4c-fb0a-c9c8-6438-144cfa77fd4b@linux.ibm.com> <814d768f-b858-f377-0155-fdefed0aa078@linux.ibm.com> In-Reply-To: <814d768f-b858-f377-0155-fdefed0aa078@linux.ibm.com> From: Richard Biener Date: Thu, 23 Nov 2023 09:20:20 +0100 Message-ID: Subject: Re: PING^1 [PATCH v3] sched: Change no_real_insns_p to no_real_nondebug_insns_p [PR108273] To: "Kewen.Lin" Cc: Alexander Monakov , Jeff Law , Maxim Kuvyrkov , GCC Patches , Richard Sandiford , Jeff Law , Vladimir Makarov , zhroma@ispras.ru, Andrey Belevantsev , Segher Boessenkool , Peter Bergner , Michael Meissner , Alexandre Oliva Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-7.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,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 Thu, Nov 23, 2023 at 4:02=E2=80=AFAM Kewen.Lin wro= te: > > on 2023/11/22 18:25, Richard Biener wrote: > > On Wed, Nov 22, 2023 at 10:31=E2=80=AFAM Kewen.Lin wrote: > >> > >> on 2023/11/17 20:55, Alexander Monakov wrote: > >>> > >>> On Fri, 17 Nov 2023, Kewen.Lin wrote: > >>>>> I don't think you can run cleanup_cfg after sched_init. I would sug= gest > >>>>> to put it early in schedule_insns. > >>>> > >>>> Thanks for the suggestion, I placed it at the beginning of haifa_sch= ed_init > >>>> instead, since schedule_insns invokes haifa_sched_init, although the > >>>> calls rgn_setup_common_sched_info and rgn_setup_sched_infos are exec= uted > >>>> ahead but they are all "setup" functions, shouldn't affect or be aff= ected > >>>> by this placement. > >>> > >>> I was worried because sched_init invokes df_analyze, and I'm not sure= if > >>> cfg_cleanup can invalidate it. > >> > >> Thanks for further explaining! By scanning cleanup_cfg, it seems that= it > >> considers df, like compact_blocks checks df, try_optimize_cfg invokes > >> df_analyze etc., but I agree that moving cleanup_cfg before sched_init > >> makes more sense. > >> > >>> > >>>>> I suspect this may be caused by invoking cleanup_cfg too late. > >>>> > >>>> By looking into some failures, I found that although cleanup_cfg is = executed > >>>> there would be still some empty blocks left, by analyzing a few fail= ures there > >>>> are at least such cases: > >>>> 1. empty function body > >>>> 2. block holding a label for return. > >>>> 3. block without any successor. > >>>> 4. block which becomes empty after scheduling some other block. > >>>> 5. block which looks mergeable with its always successor but left. > >>>> ... > >>>> > >>>> For 1,2, there is one single successor EXIT block, I think they don'= t affect > >>>> state transition, for 3, it's the same. For 4, it depends on if we = can have > >>>> the assumption this kind of empty block doesn't have the chance to h= ave debug > >>>> insn (like associated debug insn should be moved along), I'm not sur= e. For 5, > >>>> a reduced test case is: > >>> > >>> Oh, I should have thought of cases like these, really sorry about the= slip > >>> of attention, and thanks for showing a testcase for item 5. As Richar= d as > >>> saying in his response, cfg_cleanup cannot be a fix here. The thing t= o check > >>> would be changing no_real_insns_p to always return false, and see if = the > >>> situation looks recoverable (if it breaks bootstrap, regtest statisti= cs of > >>> a non-bootstrapped compiler are still informative). > >> > >> As you suggested, I forced no_real_insns_p to return false all the tim= e, some > >> issues got exposed, almost all of them are asserting NOTE_P insn shoul= dn't be > >> encountered in those places, so the adjustments for most of them are j= ust to > >> consider NOTE_P or this kind of special block and so on. One draft pa= tch is > >> attached, it can be bootstrapped and regress-tested on ppc64{,le} and = x86. > >> btw, it's without the previous cfg_cleanup adjustment (hope it can get= more > >> empty blocks and expose more issues). The draft isn't qualified for c= ode > >> review but I hope it can provide some information on what kinds of cha= nges > >> are needed for the proposal. If this is the direction which we all ag= ree on, > >> I'll further refine it and post a formal patch. One thing I want to n= ote is > >> that this patch disable one assertion below: > >> > >> diff --git a/gcc/sched-rgn.cc b/gcc/sched-rgn.cc > >> index e5964f54ead..abd334864fb 100644 > >> --- a/gcc/sched-rgn.cc > >> +++ b/gcc/sched-rgn.cc > >> @@ -3219,7 +3219,7 @@ schedule_region (int rgn) > >> } > >> > >> /* Sanity check: verify that all region insns were scheduled. */ > >> - gcc_assert (sched_rgn_n_insns =3D=3D rgn_n_insns); > >> + // gcc_assert (sched_rgn_n_insns =3D=3D rgn_n_insns); > >> > >> sched_finish_ready_list (); > >> > >> Some cases can cause this assertion to fail, it's due to the mismatch = on > >> to-be-scheduled and scheduled insn counts. The reason why it happens = is that > >> one block previously has only one INSN_P but while scheduling some oth= er blocks > >> it gets moved as well then we ends up with an empty block so that the = only > >> NOTE_P insn was counted then, but since this block isn't empty initial= ly and > >> NOTE_P gets skipped in a normal block, the count to-be-scheduled can't= count > >> it in. It can be fixed with special-casing this kind of block for cou= nting > >> like initially recording which block is empty and if a block isn't rec= orded > >> before then fix up the count for it accordingly. I'm not sure if some= one may > >> have an argument that all the complication make this proposal beaten b= y > >> previous special-casing debug insn approach, looking forward to more c= omments. > > > > Just a comment that the NOTE_P thing is odd - do we only ever have thos= e for > > otherwise empty BBs? How are they skipped otherwise (and why does that= not > > work for otherwise empty BBs)? > > Yes, previously (bypassing empty BBs) there is no chance to encounter NOT= E_P > when scheduling insns, as for notes in normal BBs, when setting up the he= ad > and tail, some are skipped (like get_ebb_head_tail), and there are also s= ome > special handlings remove_notes and unlink_bb_notes to guarantee they are > gone. By disabling empty BB bypassing, all empty BBs will be actually > uniformed as (head =3D=3D tail && NOTE_P (head)), we have to deal with NO= TE_P. I see. I expected most of them to be naturally part of another EBB. So it= 's rather a limitation of the head/tail representation. I wonder if there's a more minimal fix though. But iff head or tail of an EBB then I guess either head or tail has to point to a stmt in said block which necess= arily then means either a debug or note. Richard. > > BR, > Kewen