On Mon, Apr 28, 2025 at 1:23 AM Richard Biener
<richard.guent...@gmail.com> wrote:
>
> On Sun, Apr 27, 2025 at 1:07 AM Andrew Pinski <quic_apin...@quicinc.com>
> wrote:
> >
> > When we have an empty function, things can go wrong with
> > cfi_startproc/cfi_endproc and a few other
> > things like exceptions. So if the only thing the function does is a call to
> > __builtin_unreachable,
> > let's expand that to a __builtin_trap instead. For most targets that is one
> > instruction wide so it
> > won't hurt things that much and we get correct behavior for exceptions and
> > some linkers will be better
> > for it.
>
> Doing this at RTL expansion is a bit of an awkward spot since it never
> appears in any dump.
> We have pass_cleanup_cfg_post_optimizing aka ".optimized", so I wonder
> if we could do
> this there instead?
Yes that might be a good place to put this.
>
> From a QOI perspective we'd really want to turn any unreachable() that
> falls off the function
> into a trap (also consider hot/cold partitioning here). ISTR we
> talked about all this in detail
> somewhen back, but I don't remember the issues with doing this after BB
> reorder.
That would be a good idea too. Let me think about it for a week to see
if it is an easy way of implementing that. The only issue is not all
targets have a trap instruction.
E.g. avr, bpf, c6x, epiphany, fr30, frv, ft32, h8300, lm32, m32c,
m32r, mcore, mmix, mn10300, moxie, msp430, or1k, pdp11, pru, rl78, rx,
(and some others) don't have a trap instruction defined.
I am thinking I should only do this conversion in the first place if
there is a trap instruction. E.g. for BPF some code will be rejected
as abort is not really callable.
Anyways let me think about this some more.
Thanks,
Andrew
.
>
> Richard.
>
> > Bootstrapped and tested on x86_64-linux-gnu.
> >
> > PR middle-end/109267
> >
> > gcc/ChangeLog:
> >
> > * cfgexpand.cc (expand_gimple_basic_block): If the first non debug
> > statement in the
> > first (and only) basic block is a call to __builtin_unreachable
> > change it to a call
> > to __builtin_trap.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.dg/pr109267-1.c: New test.
> > * gcc.dg/pr109267-2.c: New test.
> >
> > Signed-off-by: Andrew Pinski <quic_apin...@quicinc.com>
> > ---
> > gcc/cfgexpand.cc | 8 ++++++++
> > gcc/testsuite/gcc.dg/pr109267-1.c | 14 ++++++++++++++
> > gcc/testsuite/gcc.dg/pr109267-2.c | 13 +++++++++++++
> > 3 files changed, 35 insertions(+)
> > create mode 100644 gcc/testsuite/gcc.dg/pr109267-1.c
> > create mode 100644 gcc/testsuite/gcc.dg/pr109267-2.c
> >
> > diff --git a/gcc/cfgexpand.cc b/gcc/cfgexpand.cc
> > index e84f12a5e93..e14df760b7a 100644
> > --- a/gcc/cfgexpand.cc
> > +++ b/gcc/cfgexpand.cc
> > @@ -6206,6 +6206,14 @@ expand_gimple_basic_block (basic_block bb, bool
> > disable_tail_calls)
> > basic_block new_bb;
> >
> > stmt = gsi_stmt (gsi);
> > +
> > + /* If we are expanding the first (and only) bb and the only non debug
> > + statement is __builtin_unreachable call, then replace it with a
> > trap
> > + so the function is at least one instruction in size. */
> > + if (!nondebug_stmt_seen && bb->index == NUM_FIXED_BLOCKS
> > + && gimple_call_builtin_p (stmt, BUILT_IN_UNREACHABLE))
> > + gimple_call_set_fndecl(stmt, builtin_decl_implicit (BUILT_IN_TRAP));
> > +
> > if (!is_gimple_debug (stmt))
> > nondebug_stmt_seen = true;
> >
> > diff --git a/gcc/testsuite/gcc.dg/pr109267-1.c
> > b/gcc/testsuite/gcc.dg/pr109267-1.c
> > new file mode 100644
> > index 00000000000..4f1da8b41e3
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/pr109267-1.c
> > @@ -0,0 +1,14 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -fdump-rtl-expand-details" } */
> > +
> > +/* PR middle-end/109267 */
> > +
> > +int f(void)
> > +{
> > + __builtin_unreachable();
> > +}
> > +
> > +/* This unreachable should expand as trap. */
> > +
> > +/* { dg-final { scan-rtl-dump-times "__builtin_trap " 1 "expand"} } */
> > +/* { dg-final { scan-rtl-dump-times "__builtin_unreachable " 1 "expand"} }
> > */
> > diff --git a/gcc/testsuite/gcc.dg/pr109267-2.c
> > b/gcc/testsuite/gcc.dg/pr109267-2.c
> > new file mode 100644
> > index 00000000000..e6da4860998
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/pr109267-2.c
> > @@ -0,0 +1,13 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -fdump-rtl-expand-details" } */
> > +
> > +/* PR middle-end/109267 */
> > +void g(void);
> > +int f(int *t)
> > +{
> > + g();
> > + __builtin_unreachable();
> > +}
> > +
> > +/* This should be expanded to unreachable so it should show up twice. */
> > +/* { dg-final { scan-rtl-dump-times "__builtin_unreachable " 2 "expand"} }
> > */
> > --
> > 2.43.0
> >
