[Bug rtl-optimization/93264] [10 Regression] ICE in cfg_layout_redirect_edge_and_branch_force, at cfgrtl.c:4522

[zhroma at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93264

Roman Zhuykov <zhroma at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |abel at gcc dot gnu.org

--- Comment #2 from Roman Zhuykov <zhroma at gcc dot gnu.org> ---
So I decide to share my thoughts here to prevent silently dropping this to P2
before gcc-10 release.  Some of the following is already presented in meta-bug
(PR90040).

(In reply to Jakub Jelinek from comment #1)
> It tries to redirect a crossing jump from cold to hot partition to a new bb
> that is in cold partition.
> The generic code really doesn't know what it should remove to replace it
> with the fallthru edge.
Thank you for looking at it.  In 2018 Andrey helps me with looking at PR85408
and PR85426 and we together came to the same conclusion.  That time we decide
RTL folks will eventually look at those bugs and find some solution.  A year
later I've opened a meta-bug, and now in 2020 we still have those issues.

> cfg_layout_redirect_edge_and_branch has code to handle this case, but only
> if the 
> branch is simplejump:
> 4429	  if (e->flags & EDGE_CROSSING
> 4430	      && BB_PARTITION (e->src) == BB_PARTITION (dest)
> 4431	      && simplejump_p (BB_END (src)))
> 4432	    {
> 4433	      if (dump_file)
> 4434		fprintf (dump_file,
> 4435		  	 "Removing crossing jump while redirecting edge form %i to %i\n",
> 4436			 e->src->index, dest->index);
> 4437	      delete_insn (BB_END (src));
> 4438	      remove_barriers_from_footer (src);
> 4439	      e->flags |= EDGE_FALLTHRU;
> 4440	    }
Yep, and this code is rather new, it was also added in 2018 half a year before
PR87475 fix.  I mentioned the quoted hunk in meta-bug discussion in [Appendix
1].  Although Honza created that patch as a fix for PR84058 which was open as
code quality issue, same hunk also solved an ICE in PR83886.

Bug 84058 comment 9 lefts that PR open to track enhancements, but hopefully ICE
 issues will also have some attention.

> So, I'd say something in the caller or a few callers up should have punted
> on it before, but no idea what.
Quoting bug 85408 comment 2: "Honza, thoughts on this?" :)

Here I will also desribe some alternative ways to fix the issue, from simplest
to hardest. IMHO all those ideas are ugly.

(1) We can prohibit usage of -freorder-blocks-and-partition and -fmodulo-sched
together.  Maybe it should issue a note and enable only the pass which goes
last in compile options.  This behaviour will be much more confusing when my
sms changes goes in, which will allow to use sms on x86, where partitioning is
enabled by default.

(2) We can move pass_sms before pass_partition_blocks.  Althougt this will
eventually prevent another entering/exiting cfg_layout mode, it is a bad idea. 
It was discussed previously (see meta-bug), probably sms will become less
accurate.  It's better to keep sms pass as closer as possible to register
allocation pass.

(3) As mentioned in meta-bug [Appendix 2] we probably have some latent issues
with entering cfg_layout.  Sms doesn't guard entering, while all other passes
which run after partitioning have some logic like this (hw-doloop.c):
  /* We can't enter cfglayout mode anymore if basic block partitioning
     already happened.  */
  if (do_reorder && !crtl->has_bb_partition)
  ...
SMS wants loop_optimizer_init, and it may call loop_version when succeeded, so
it actually can't proceed without entering cfg_layout.  If we add a guard like
that, we will stop sms to process any function where has_bb_partition is true. 
Mostly this have same problems as solution (1), but it will also silently
prioritize partitioning over sms instead of noting the user.

(4) We can add another pass_sms item in passes.def before pass_partition_blocks
and add some confusing logic: when paritioning is enabled, sms will run
earlier. Certainly, it will run as usual otherwise.

(5) Approach (4) can be extended if we somehow split pass_partition_blocks into
separate 'analyze' and 'change_cfg' phases. Than "earlier sms" should take
effect only when 'analyze' is successfull.  And certainly as "earlier sms" will
itself change the cfg, we than have to re-run 'analyze' again.