Re: Do not account __builtin_unreachable guards in inliner

[Jan Hubicka <hubicka@ucw.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 ...

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.

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?
We could also perhaps declare unreachable NOVOPs which would make DSE to
remove the stores.

> 
> ... 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.

Honza