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 13:30:47 +0200	[thread overview]
Message-ID: <CAFiYyc3k0O5Ah0q=2j=81=JYcGQzwUa-BV=GOG5nGrFih=54=Q@mail.gmail.com> (raw)
In-Reply-To: <ZJAqxK3h4ncImWcD@kam.mff.cuni.cz>

On Mon, Jun 19, 2023 at 12:15 PM Jan Hubicka <hubicka@ucw.cz> wrote:
>
> > 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 ...
>
> I was thinking of this: it depends on how smart do we want to get.
> We also have dead conditionals guarding clobbers, predicts and other
> stuff.  In general we can use mark phase of cd-dce telling it to ignore
> those statements and then use its resut in the analysis.

Hmm, possible but a bit heavy-handed.  There's simple_dce_from_worklist
which might be a way to do this (of course we cannot use that 1:1).  Also
then consider

 a = a + 1;
 if (a > 10)
   __builtin_unreachable ();
 if (a < 5)
   __builtin_unreachable ();

and a has more than one use but both are going away.  So indeed a
more global analysis would be needed to get the full benefit.

> Also question is how much we can rely on middle-end optimizing out
> unreachables.  For example:
> int a;
> int b[3];
> test()
> {
>   if (a > 0)
>     {
>       for (int i = 0; i < 3; i++)
>           b[i] = 0;
>       __builtin_unreachable ();
>     }
> }
>
> IMO can be optimized to empty function.  I believe we used to have code
> in tree-cfgcleanup to remove statements just before
> __builtin_unreachable which can not terminate execution, but perhaps it
> existed only in my local tree?

I think we rely on DCE/DSE here and explicit unreachable () pruning after
VRP picked up things (I think it simply gets the secondary effect optimizing
the condition it created the range for in the first pass).

DSE is appearantly not able to kill the stores, I will fix that.  I
think DCE can,
but only for non-aliased stores.

> We could also perhaps declare unreachable NOVOPs which would make DSE to
> remove the stores.

But only because of a bug in DSE ... it also removes them if that
__builtin_unreachable ()
is GIMPLE_RESX.

> >
> > ... we do code generate BUILT_IN_UNREACHABLE_TRAP, no?
>
> You are right.  I tested it with -funreachable-traps but it does not do
> what I expected, I need -fsanitize=unreachable -fsanitize-trap=unreachable
>
> Also if I try to call it by hand I get:
>
> jan@localhost:/tmp> gcc t.c -S -O2 -funreachable-traps -fdump-tree-all-details -fsanitize=unreachable -fsanitize-trap=unreachable
> t.c: In function ‘test’:
> t.c:9:13: warning: implicit declaration of function ‘__builtin_unreachable_trap’; did you mean ‘__builtin_unreachable trap’? [-Wimplicit-function-declaration]
>     9 |             __builtin_unreachable_trap ();
>       |             ^~~~~~~~~~~~~~~~~~~~~~~~~~
>       |             __builtin_unreachable trap
>
> Which is not as helpful as it is trying to be.
> >
> > > +    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.
>
> I did not know about the chunk API which is certainly nice :)
> sbitmap will always allocate, while here we stay on stack for small
> functions and I am not sure how much extra bit operations would not
> offset extra memset, but overall I think it is all in noise.

Ah, yeah.
> Honza

  reply	other threads:[~2023-06-19 11:33 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
2023-06-19 10:15   ` Jan Hubicka
2023-06-19 11:30     ` Richard Biener [this message]
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='CAFiYyc3k0O5Ah0q=2j=81=JYcGQzwUa-BV=GOG5nGrFih=54=Q@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).