public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Richard Biener <richard.guenther@gmail.com>
To: Jan Hubicka <hubicka@ucw.cz>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: Do not account __builtin_unreachable guards in inliner
Date: Mon, 19 Jun 2023 11:01:41 +0200	[thread overview]
Message-ID: <CAFiYyc2iZKEQC1ickKToXVratX-ZVF7dzQ=vvwcC9RvZbtq10w@mail.gmail.com> (raw)
In-Reply-To: <ZJAJMPiUB4oxqZBR@kam.mff.cuni.cz>

On Mon, Jun 19, 2023 at 9:52 AM Jan Hubicka via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi,
> this was suggested earlier somewhere, but I can not find the thread.
> C++ has assume attribute that expands int
>   if (conditional)
>     __builtin_unreachable ()
> We do not want to account the conditional in inline heuristics since
> we know that it is going to be optimized out.
>
> Bootstrapped/regtested x86_64-linux, will commit it later today if
> thre are no complains.

I think we also had the request to not account the condition feeding
stmts (if they only feed it and have no side-effects).  libstdc++ has
complex range comparisons here.  Also ...

> gcc/ChangeLog:
>
>         * ipa-fnsummary.cc (builtin_unreachable_bb_p): New function.
>         (analyze_function_body): Do not account conditionals guarding
>         builtin_unreachable calls.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.dg/ipa/fnsummary-1.c: New test.
>
> diff --git a/gcc/ipa-fnsummary.cc b/gcc/ipa-fnsummary.cc
> index a5f5a50c8a5..987da29ec34 100644
> --- a/gcc/ipa-fnsummary.cc
> +++ b/gcc/ipa-fnsummary.cc
> @@ -2649,6 +2649,54 @@ points_to_possible_sra_candidate_p (tree t)
>    return false;
>  }
>
> +/* Return true if BB is builtin_unreachable.
> +   We skip empty basic blocks, debug statements, clobbers and predicts.
> +   CACHE is used to memoize already analyzed blocks.  */
> +
> +static bool
> +builtin_unreachable_bb_p (basic_block bb, vec<unsigned char> &cache)
> +{
> +  if (cache[bb->index])
> +    return cache[bb->index] - 1;
> +  gimple_stmt_iterator si;
> +  auto_vec <basic_block, 4> visited_bbs;
> +  bool ret = false;
> +  while (true)
> +    {
> +      bool empty_bb = true;
> +      visited_bbs.safe_push (bb);
> +      cache[bb->index] = 3;
> +      for (si = gsi_start_nondebug_bb (bb);
> +          !gsi_end_p (si) && empty_bb;
> +          gsi_next_nondebug (&si))
> +       {
> +         if (gimple_code (gsi_stmt (si)) != GIMPLE_PREDICT
> +             && !gimple_clobber_p (gsi_stmt (si))
> +             && !gimple_nop_p (gsi_stmt (si)))
> +           {
> +             empty_bb = false;
> +             break;
> +           }
> +       }
> +      if (!empty_bb)
> +       break;
> +      else
> +       bb = single_succ_edge (bb)->dest;
> +      if (cache[bb->index])
> +       {
> +         ret = cache[bb->index] == 3 ? false : cache[bb->index] - 1;
> +         goto done;
> +       }
> +    }
> +  if (gimple_call_builtin_p (gsi_stmt (si), BUILT_IN_UNREACHABLE)
> +      || gimple_call_builtin_p (gsi_stmt (si), BUILT_IN_UNREACHABLE_TRAP))

... we do code generate BUILT_IN_UNREACHABLE_TRAP, no?

> +    ret = true;
> +done:
> +  for (basic_block vbb:visited_bbs)
> +    cache[vbb->index] = (unsigned char)ret + 1;
> +  return ret;
> +}
> +
>  /* Analyze function body for NODE.
>     EARLY indicates run from early optimization pipeline.  */
>
> @@ -2743,6 +2791,8 @@ analyze_function_body (struct cgraph_node *node, bool early)
>    const profile_count entry_count = ENTRY_BLOCK_PTR_FOR_FN (cfun)->count;
>    order = XNEWVEC (int, n_basic_blocks_for_fn (cfun));
>    nblocks = pre_and_rev_post_order_compute (NULL, order, false);
> +  auto_vec<unsigned char, 10> cache;
> +  cache.safe_grow_cleared (last_basic_block_for_fn (cfun));

A sbitmap with two bits per entry would be more space efficient here.  bitmap
has bitmap_set_aligned_chunk and bitmap_get_aligned_chunk for convenience,
adding the corresponding to sbitmap.h would likely ease use there as well.

>    for (n = 0; n < nblocks; n++)
>      {
>        bb = BASIC_BLOCK_FOR_FN (cfun, order[n]);
> @@ -2901,6 +2951,24 @@ analyze_function_body (struct cgraph_node *node, bool early)
>                 }
>             }
>
> +         /* Conditionals guarding __builtin_unreachable will be
> +            optimized out.  */
> +         if (gimple_code (stmt) == GIMPLE_COND)
> +           {
> +             edge_iterator ei;
> +             edge e;
> +             FOR_EACH_EDGE (e, ei, bb->succs)
> +               if (builtin_unreachable_bb_p (e->dest, cache))
> +                 {
> +                   if (dump_file)
> +                     fprintf (dump_file,
> +                              "\t\tConditional guarding __builtin_unreachable; ignored\n");
> +                   this_time = 0;
> +                   this_size = 0;
> +                   break;
> +                 }
> +           }
> +
>           /* TODO: When conditional jump or switch is known to be constant, but
>              we did not translate it into the predicates, we really can account
>              just maximum of the possible paths.  */
> diff --git a/gcc/testsuite/gcc.dg/ipa/fnsummary-1.c b/gcc/testsuite/gcc.dg/ipa/fnsummary-1.c
> new file mode 100644
> index 00000000000..a0ece0c300b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/ipa/fnsummary-1.c
> @@ -0,0 +1,8 @@
> +/* { dg-options "-O2 -fdump-ipa-fnsummary"  } */
> +int
> +test(int a)
> +{
> +       if (a > 10)
> +               __builtin_unreachable ();
> +}
> +/* { dg-final { scan-ipa-dump "Conditional guarding __builtin_unreachable" "fnsummary"  } } */

  reply	other threads:[~2023-06-19  9:04 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-06-19  7:52 Jan Hubicka
2023-06-19  9:01 ` Richard Biener [this message]
2023-06-19 10:15   ` Jan Hubicka
2023-06-19 11:30     ` Richard Biener
2023-06-19 11:40       ` Richard Biener
2023-06-23 10:11       ` Jan Hubicka
2023-06-23 11:11         ` Richard Biener
2023-06-23 13:19           ` Jan Hubicka

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='CAFiYyc2iZKEQC1ickKToXVratX-ZVF7dzQ=vvwcC9RvZbtq10w@mail.gmail.com' \
    --to=richard.guenther@gmail.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=hubicka@ucw.cz \
    /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).