From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x129.google.com (mail-lf1-x129.google.com [IPv6:2a00:1450:4864:20::129]) by sourceware.org (Postfix) with ESMTPS id D82273858D35 for ; Wed, 22 Nov 2023 10:29:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D82273858D35 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 D82273858D35 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::129 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1700648948; cv=none; b=RmFuxQDEJ3Jh8SN6yRSQsunVgf/MPAA7EsjxcON/3HvLdRRzoRhVbOa5qRbHi2MesJlmz0B5z0rvskYyhIiiCXqjBFjhvyXlJVj2i8o/o82k/vue/rR7A0Vt4sJopPUu8RaAx+dqWCGrCsIUI/THZBKyqULckCigBIidy41MIps= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1700648948; c=relaxed/simple; bh=7dlZAaXhq1Amc7cX4vrO+jecPtN8kK4/HSg5QipIRaI=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=n3Nau8eq8NcEVjyXiBj5stoonptqKWunLVs60ynhVoxCYXJyJjWUtZVGY/jm4bIyV4mnAz/R1/hrRwDgJ3xXz8xkHrCONBBXXSS9xDYFJrMGQY341WoqNOHBaXNRmw6dsnaKcQl4XzR4QAyIsqmfBFSUNbh9DxLcR/PC0X+uTBc= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-lf1-x129.google.com with SMTP id 2adb3069b0e04-507a62d4788so9351761e87.0 for ; Wed, 22 Nov 2023 02:29:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1700648945; x=1701253745; 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=Ku0ORezzUM9Vp8/McNGgJNsBGtaqg97xDHzZjMJlVHs=; b=GjShxNSRiyd/FqWObnw6DgBrayznzJ/kjhRBktzVcVrvuGIrDCOZeYzIs6bakMxwkI vBLZnt40ej6yuJG5kcwFQaALrNjFgsxnQwzpIlNVD77ElgrwvhDcLYCcLUmINdLe2QmW 5yeX1qQFB6Psc3Tw7u7x18zSbE/Pi9JzQ5ee5nPuKCMxY6Ui95KqjALLwI0f9Rj41IKy GsoJv3VRqaDZt84WGWgAlokngL5Z0gfqI2P/f+u19GAWG+iWZO+05Vi2zO0LIBSgHl0f q0jf5xGuJLqPt17o8KiOjyVFP7+kWiJOjduTRkcjVN9/6BbaaRC5IVFERPVEaNqhGv6t Y8bg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700648945; x=1701253745; 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=Ku0ORezzUM9Vp8/McNGgJNsBGtaqg97xDHzZjMJlVHs=; b=fsBiCNslUBthLYq/ldlvprqmsPY4PwqzUG1FLMT5X+nFi9SkYp/0m1utWe12pa4x/H N2saBnqPyqfBwJR47OmGnTIW5i8OLfY7N7OoJ2mgtCo00GRYHwC/iXoK9CxlGa7ZdO5q TAOblGNBt71a7SCvmqqmxF+k2HyvlgEYR8TyKa9hZtHvy5vY/g1cHvodKR6i5YcVhTm8 0MqOFQqbxW0vXorwHztfmSS8ptpuD6GXc5Do1GaEomxL5Ywc8QSX0/u/ph40PJeMZBT7 WXMjrob6yt3IqxnMHTQiFuzvhWuu1ZIF4AxqExbyVIyD2f+dbGsEaITROtIsr1IX6SMB fkMw== X-Gm-Message-State: AOJu0Yxo9P00zMMsv5oQZ0Fui2ZeBdhCAr/ZzoIFbqhx5gURAJZpTKv/ Ng0rjh5r/60SlLZ9LLJ7knf1dCPv0+3fP4O0nTs= X-Google-Smtp-Source: AGHT+IGgOIEZ7ZiObdLE22ILvtzFyEVmJFbwrq+LTdDKDbWUJXh1I7y/arndchELB72t3i9FPn8aMKdqV4CfcSC70mo= X-Received: by 2002:ac2:554d:0:b0:507:9787:6776 with SMTP id l13-20020ac2554d000000b0050797876776mr1250456lfk.5.1700648944932; Wed, 22 Nov 2023 02:29:04 -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> In-Reply-To: <1f407c4c-fb0a-c9c8-6438-144cfa77fd4b@linux.ibm.com> From: Richard Biener Date: Wed, 22 Nov 2023 11:25:23 +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.3 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 Wed, Nov 22, 2023 at 10:31=E2=80=AFAM Kewen.Lin wr= ote: > > 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 sugge= st > >>> to put it early in schedule_insns. > >> > >> Thanks for the suggestion, I placed it at the beginning of haifa_sched= _init > >> instead, since schedule_insns invokes haifa_sched_init, although the > >> calls rgn_setup_common_sched_info and rgn_setup_sched_infos are execut= ed > >> ahead but they are all "setup" functions, shouldn't affect or be affec= ted > >> by this placement. > > > > I was worried because sched_init invokes df_analyze, and I'm not sure i= f > > 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 ex= ecuted > >> there would be still some empty blocks left, by analyzing a few failur= es 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 ca= n have > >> the assumption this kind of empty block doesn't have the chance to hav= e debug > >> insn (like associated debug insn should be moved along), I'm not sure.= For 5, > >> a reduced test case is: > > > > Oh, I should have thought of cases like these, really sorry about the s= lip > > of attention, and thanks for showing a testcase for item 5. As Richard = as > > saying in his response, cfg_cleanup cannot be a fix here. The thing to = check > > would be changing no_real_insns_p to always return false, and see if th= e > > situation looks recoverable (if it breaks bootstrap, regtest statistics= of > > a non-bootstrapped compiler are still informative). > > As you suggested, I forced no_real_insns_p to return false all the time, = some > issues got exposed, almost all of them are asserting NOTE_P insn shouldn'= t be > encountered in those places, so the adjustments for most of them are just= to > consider NOTE_P or this kind of special block and so on. One draft patch= 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 mo= re > empty blocks and expose more issues). The draft isn't qualified for code > review but I hope it can provide some information on what kinds of change= s > are needed for the proposal. If this is the direction which we all agree= on, > I'll further refine it and post a formal patch. One thing I want to note= 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 other = blocks > it gets moved as well then we ends up with an empty block so that the onl= y > NOTE_P insn was counted then, but since this block isn't empty initially = and > NOTE_P gets skipped in a normal block, the count to-be-scheduled can't co= unt > it in. It can be fixed with special-casing this kind of block for counti= ng > like initially recording which block is empty and if a block isn't record= ed > before then fix up the count for it accordingly. I'm not sure if someone= may > have an argument that all the complication make this proposal beaten by > previous special-casing debug insn approach, looking forward to more comm= ents. Just a comment that the NOTE_P thing is odd - do we only ever have those fo= r otherwise empty BBs? How are they skipped otherwise (and why does that not work for otherwise empty BBs)? Richard. > BR, > Kewen >