public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jakub Jelinek <jakub@redhat.com>
To: Jan Hubicka <jh@suse.cz>, Richard Biener <rguenther@suse.de>
Cc: gcc-patches@gcc.gnu.org
Subject: [PATCH] bb-reorder: Fix -freorder-blocks-and-partition ICEs on aarch64 with asm goto [PR110079]
Date: Thu, 7 Mar 2024 09:22:36 +0100	[thread overview]
Message-ID: <Zel5TMMr/3BHgl0g@tucnak> (raw)

Hi!

The following testcase ICEs, because fix_crossing_unconditional_branches
thinks that asm goto is an unconditional jump and removes it, replacing it
with unconditional jump to one of the labels.
This doesn't happen on x86 because the function in question isn't invoked
there at all:
  /* If the architecture does not have unconditional branches that
     can span all of memory, convert crossing unconditional branches
     into indirect jumps.  Since adding an indirect jump also adds
     a new register usage, update the register usage information as
     well.  */
  if (!HAS_LONG_UNCOND_BRANCH)
    fix_crossing_unconditional_branches ();
I think for the asm goto case, for the non-fallthru edge if any we should
handle it like any other fallthru (and fix_crossing_unconditional_branches
doesn't really deal with those, it only looks at explicit branches at the
end of bbs and we are in cfglayout mode at that point) and for the labels
we just pass the labels as immediates to the assembly and it is up to the
user to figure out how to store them/branch to them or whatever they want to
do.
So, the following patch fixes this by not treating asm goto as a simple
unconditional jump.

I really think that on the !HAS_LONG_UNCOND_BRANCH targets we have a bug
somewhere else, where outofcfglayout or whatever should actually create
those indirect jumps on the crossing edges instead of adding normal
unconditional jumps, I see e.g. in
__attribute__((cold)) int bar (char *);
__attribute__((hot)) int baz (char *);
void qux (int x) { if (__builtin_expect (!x, 1)) goto l1; bar (""); goto l1; l1: baz (""); }
void corge (int x) { if (__builtin_expect (!x, 0)) goto l1; baz (""); l2: return; l1: bar (""); goto l2; }
with -O2 -freorder-blocks-and-partition on aarch64 before/after this patch
just b .L? jumps which I believe are +-32MB, so if .text is larger than
32MB, it could fail to link, but this patch doesn't address that.

Bootstrapped/regtested on x86_64-linux, i686-linux and aarch64-linux, ok for
trunk?

2024-03-07  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/110079
	* bb-reorder.cc (fix_crossing_unconditional_branches): Don't adjust
	asm goto.

	* gcc.dg/pr110079.c: New test.

--- gcc/bb-reorder.cc.jj	2024-01-03 11:51:32.000000000 +0100
+++ gcc/bb-reorder.cc	2024-03-06 18:34:29.468016144 +0100
@@ -2266,7 +2266,8 @@ fix_crossing_unconditional_branches (voi
 	  /* Make sure the jump is not already an indirect or table jump.  */
 
 	  if (!computed_jump_p (last_insn)
-	      && !tablejump_p (last_insn, NULL, NULL))
+	      && !tablejump_p (last_insn, NULL, NULL)
+	      && !asm_noperands (PATTERN (last_insn)))
 	    {
 	      /* We have found a "crossing" unconditional branch.  Now
 		 we must convert it to an indirect jump.  First create
--- gcc/testsuite/gcc.dg/pr110079.c.jj	2024-03-06 18:42:47.175250069 +0100
+++ gcc/testsuite/gcc.dg/pr110079.c	2024-03-06 18:44:47.008620726 +0100
@@ -0,0 +1,43 @@
+/* PR rtl-optimization/110079 */
+/* { dg-do compile { target lra } } */
+/* { dg-options "-O2" } */
+/* { dg-additional-options "-freorder-blocks-and-partition" { target freorder } } */
+
+int a;
+__attribute__((cold)) int bar (char *);
+__attribute__((hot)) int baz (char *);
+
+void
+foo (void)
+{
+l1:
+  while (a)
+    ;
+  bar ("");
+  asm goto ("" : : : : l2);
+  asm ("");
+l2:
+  goto l1;
+}
+
+void
+qux (void)
+{
+  asm goto ("" : : : : l1);
+  bar ("");
+  goto l1;
+l1:
+  baz ("");
+}
+
+void
+corge (void)
+{
+  asm goto ("" : : : : l1);
+  baz ("");
+l2:
+  return;
+l1:
+  bar ("");
+  goto l2;
+}

	Jakub


             reply	other threads:[~2024-03-07  8:22 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-07  8:22 Jakub Jelinek [this message]
2024-03-07  8:52 ` Richard Biener

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Zel5TMMr/3BHgl0g@tucnak \
    --to=jakub@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jh@suse.cz \
    --cc=rguenther@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).