diff mbox series

[take,2] PR tree-optimization/38943: Preserve trapping instructions with -fpreserve-traps

Message ID 026901d7755c$e1080840$a31818c0$@nextmovesoftware.com
State New
Headers show
Series [take,2] PR tree-optimization/38943: Preserve trapping instructions with -fpreserve-traps | expand

Commit Message

Roger Sayle July 10, 2021, 7:26 a.m. UTC 
Hi Richard and Eric,
Of course, you're both completely right.  Rather than argue that
-fnon-call-exceptions without -fexceptions (and without
-fdelete-dead-exceptions) has some implicit undocumented semantics,
trapping instructions should be completely orthogonal to exception
handling.

This patch adds a new code generation option -fpreserve-traps, the
(obvious) semantics of which is demonstrated by the expanded test
case below.  The current behaviour of gcc is to eliminate calls
to may_trap_1, may_trap_2, may_trap_3 etc. from foo, but these are
all retained with -fpreserve-traps.

Historically, the semantics of -fnon-call-exceptions vs. traps has
been widely misunderstood, with different levels of optimization
producing different outcomes, as shown by the impressive list of PRs
affected by this solution.  Hopefully, this new documentation will
clarify things.

This patch has been tested on x86_64-pc-linux-gnu with a "make
bootstrap" and "make -k check" with no new failures.

Ok for mainline?


2021-07-09  Roger Sayle  <roger@nextmovesoftware.com>
	    Eric Botcazou  <ebotcazou@adacore.com>
	    Richard Biener  <rguenther@suse.de>

gcc/ChangeLog
	PR tree-optimization/38943
	PR middle-end/39801
	PR middle-end/64711
	PR target/70387
	PR tree-optimization/94357
	* common.opt (fpreserve-traps): New code generation option.
	* doc/invoke.texi (-fpreserve-traps): Document new option.
	* gimple.c (gimple_has_side_effects): Consider trapping to
	be a side-effect when -fpreserve-traps is specified.
	(gimple_could_trap_p_1):  Make S argument a "const gimple*".
	Preserve constness in call to gimple_asm_volatile_p.
	(gimple_could_trap_p): Make S argument a "const gimple*".
	* gimple.h (gimple_could_trap_p_1, gimple_could_trap_p):
	Update function prototypes.
	* ipa-pure-const.c (check_stmt): When preserving traps,
	a trapping statement should be considered a side-effect,
	so the function is neither const nor pure.

gcc/testsuite/ChangeLog
	PR tree-optimization/38943
	PR middle-end/39801
	PR middle-end/64711
	PR target/70387
	PR tree-optimization/94357
	* gcc.dg/pr38943.c: New test case.

--
Roger Sayle, PhD.
CEO and founder
NextMove Software Limited
Registered in England No. 07588305
Registered Office: Innovation Centre, 320 Cambridge Science Park, Cambridge, CB4 0WG

-----Original Message-----
From: Richard Biener <richard.guenther@gmail.com> 
Sent: 08 July 2021 11:19
To: Roger Sayle <roger@nextmovesoftware.com>; Eric Botcazou <botcazou@adacore.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] PR tree-optimization/38943: Preserve trapping instructions with -fnon-call-exceptions

On Thu, Jul 8, 2021 at 11:54 AM Roger Sayle <roger@nextmovesoftware.com> wrote:
>
>
> This patch addresses PR tree-optimization/38943 where gcc may optimize 
> away trapping instructions even when -fnon-call-exceptions is specified.
> Interestingly this only affects the C compiler (when -fexceptions is 
> not
> specified) as g++ (or -fexceptions) supports C++-style exception 
> handling, where -fnon-call-exceptions triggers the stmt_could_throw_p machinery.
> Without -fexceptions, trapping instructions aren't always considered 
> visible side-effects.

But -fnon-call-exceptions without -fexceptions doesn't make much sense, does it?  I see the testcase behaves correctly when -fexceptions is also specified.

The call vanishes in DCE because stmt_could_throw_p starts with

bool
stmt_could_throw_p (function *fun, gimple *stmt) {
  if (!flag_exceptions)
    return false;

the documentation of -fnon-call-exceptions says

Generate code that allows trapping instructions to throw exceptions.

so either the above check is wrong or -fnon-call-exceptions should imply -fexceptions (or we should diagnose missing -fexceptions)

>
> This patch fixes this in two place.  Firstly, gimple_has_side_effects 
> is tweaked such that gimple_could_trap_p is considered a side-effect 
> if the current function can throw non-call exceptions.

But exceptions are not considered side-effects - they are explicit in the IL and thus passes are supposed to check for those and preserve dead (externally) throwing stmts if not told otherwise (flag_delete_dead_exceptions).

>  And secondly,
> check_stmt in ipa-pure-const.c is tweaked such that a function 
> containing a trapping statement is considered to have a side-effect 
> with -fnon-call-exceptions, and therefore cannot be pure or const.

EH is orthogonal to pure/const, so I think that's wrong.

> Calling gimple_could_trap_p (which previously took a non-const gimple) 
> from gimple_has_side_effects (which takes a const gimple) required 
> improving the const-safety of gimple_could_trap_p (a relatively minor
> tweak) and its prototypes.  Hopefully this is considered a clean-up/ 
> improvement.

Yeah, even an obvious one I think - you can push that independently.

> This patch has been tested on x86_64-pc-linux-gnu with a "make 
> bootstrap" and "make -k check" with no new failures.  This should be 
> relatively safe, as there are no changes in behaviour unless the user 
> explicitly specifies -fnon-call-exceptions, when the C compiler then 
> behaves more like the C++/Ada compiler.
>
> Ok for mainline?
>
>
> 2021-07-08  Roger Sayle  <roger@nextmovesoftware.com>
>
> gcc/ChangeLog
>         PR tree-optimization/38943
>         * gimple.c (gimple_has_side_effects): Consider trapping to
>         be a side-effect when -fnon-call-exceptions is specified.
>         (gimple_coult_trap_p_1):  Make S argument a "const gimple*".
>         Preserve constness in call to gimple_asm_volatile_p.
>         (gimple_could_trap_p): Make S argument a "const gimple*".
>         * gimple.h (gimple_could_trap_p_1, gimple_could_trap_p):
>         Update function prototypes.
>         * ipa-pure-const.c (check_stmt): When the current function
>         can throw non-call exceptions, a trapping statement should
>         be considered a side-effect, so the function is neither
>         const nor pure.
>
> gcc/testsuite/ChangeLog
>         PR tree-optimization/38943
>         * gcc.dg/pr38943.c: New test case.
>
> Roger
> --
> Roger Sayle
> NextMove Software
> Cambridge, UK
>

/* { dg-do compile } */
/* { dg-options "-O2 -fpreserve-traps -ftrapv -fdump-tree-optimized" } */

int __attribute__((noinline)) may_trap_1 (int x) {
  return 10/x;
}

int __attribute__((noinline)) may_trap_2 (int *x) {
  return *x;
}

int __attribute__((noinline)) may_trap_3 (int *x) {
  return *x;
}

int __attribute__((noinline)) may_trap_4 (int x) {
  int too_small[5];
  return too_small[x];
}

float __attribute__((noinline)) may_trap_5 (float x) {
  return 2.0f / x;
};

int __attribute__((noinline)) may_trap_6 (int x) {
  return x + x;
};

void foo()
{
  may_trap_1 (0);  // division by zero
  may_trap_2 (0);  // invalid memory access
  may_trap_3 ((int *)1);  // mis-aligned memory access
  may_trap_4 (10);  // out-of-bounds memory access
  may_trap_5 (3.0f);  // floating-point trap
  may_trap_6 ((int)((~0u)>>1));  // integer overflow with -ftrapv
}

/* { dg-final { scan-tree-dump "may_trap_1 \\(0" "optimized" } } */
/* { dg-final { scan-tree-dump "may_trap_2 \\(0" "optimized" } } */
/* { dg-final { scan-tree-dump "may_trap_3 \\(1" "optimized" } } */
/* { dg-final { scan-tree-dump "may_trap_4 \\(10" "optimized" } } */
/* { dg-final { scan-tree-dump "may_trap_5 \\(3" "optimized" } } */
/* { dg-final { scan-tree-dump "may_trap_6 \\(\[1-9\]" "optimized" } } */

Comments

Richard Biener July 12, 2021, 11:26 a.m. UTC | #1 
On Sat, Jul 10, 2021 at 9:26 AM Roger Sayle <roger@nextmovesoftware.com> wrote:
>
>
> Hi Richard and Eric,
> Of course, you're both completely right.  Rather than argue that
> -fnon-call-exceptions without -fexceptions (and without
> -fdelete-dead-exceptions) has some implicit undocumented semantics,
> trapping instructions should be completely orthogonal to exception
> handling.
>
> This patch adds a new code generation option -fpreserve-traps, the
> (obvious) semantics of which is demonstrated by the expanded test
> case below.  The current behaviour of gcc is to eliminate calls
> to may_trap_1, may_trap_2, may_trap_3 etc. from foo, but these are
> all retained with -fpreserve-traps.
>
> Historically, the semantics of -fnon-call-exceptions vs. traps has
> been widely misunderstood, with different levels of optimization
> producing different outcomes, as shown by the impressive list of PRs
> affected by this solution.  Hopefully, this new documentation will
> clarify things.

I've reviewed the cited PRs and most of them would have
individual fixes and are not fixed by your patch, though
-fpreserve-traps would offer a workaround in some cases.

Now, -fpreserve-traps follows the unfortunate precedence of
tieing IL semantics to a (global) flag rather than to individual
stmts.  I'm not sure -fpreserve-traps is something good to offer
since on its own it looks not too useful and for making use of
it one still needs -fnon-call-exceptions [-fexceptions].

There's still the open question what -fnon-call-exceptions on its
own should do - IMHO it doesn't make sense to allow unwiding
from a trapping memory reference but not from the call it resides
in which means -fnon-call-exceptions should better enable
-fexceptions?

There's also valid points made in some of the PRs (some of which
look like dups of each other) that an asm with memory operands
should be trapping and thus throwing with -fnon-call-exceptions
even when it is not volatile and that some builtin functions like
memcpy should not be nothrow with -fnon-call-exceptions.

There's const-correctness pieces in your patch - those are OK
under the obvious rule and you might want to push them separately.

Thanks,
Richard.

>
> This patch has been tested on x86_64-pc-linux-gnu with a "make
> bootstrap" and "make -k check" with no new failures.
>
> Ok for mainline?
>
>
> 2021-07-09  Roger Sayle  <roger@nextmovesoftware.com>
>             Eric Botcazou  <ebotcazou@adacore.com>
>             Richard Biener  <rguenther@suse.de>
>
> gcc/ChangeLog
>         PR tree-optimization/38943
>         PR middle-end/39801
>         PR middle-end/64711
>         PR target/70387
>         PR tree-optimization/94357
>         * common.opt (fpreserve-traps): New code generation option.
>         * doc/invoke.texi (-fpreserve-traps): Document new option.
>         * gimple.c (gimple_has_side_effects): Consider trapping to
>         be a side-effect when -fpreserve-traps is specified.
>         (gimple_could_trap_p_1):  Make S argument a "const gimple*".
>         Preserve constness in call to gimple_asm_volatile_p.
>         (gimple_could_trap_p): Make S argument a "const gimple*".
>         * gimple.h (gimple_could_trap_p_1, gimple_could_trap_p):
>         Update function prototypes.
>         * ipa-pure-const.c (check_stmt): When preserving traps,
>         a trapping statement should be considered a side-effect,
>         so the function is neither const nor pure.
>
> gcc/testsuite/ChangeLog
>         PR tree-optimization/38943
>         PR middle-end/39801
>         PR middle-end/64711
>         PR target/70387
>         PR tree-optimization/94357
>         * gcc.dg/pr38943.c: New test case.
>
> --
> Roger Sayle, PhD.
> CEO and founder
> NextMove Software Limited
> Registered in England No. 07588305
> Registered Office: Innovation Centre, 320 Cambridge Science Park, Cambridge, CB4 0WG
>
> -----Original Message-----
> From: Richard Biener <richard.guenther@gmail.com>
> Sent: 08 July 2021 11:19
> To: Roger Sayle <roger@nextmovesoftware.com>; Eric Botcazou <botcazou@adacore.com>
> Cc: GCC Patches <gcc-patches@gcc.gnu.org>
> Subject: Re: [PATCH] PR tree-optimization/38943: Preserve trapping instructions with -fnon-call-exceptions
>
> On Thu, Jul 8, 2021 at 11:54 AM Roger Sayle <roger@nextmovesoftware.com> wrote:
> >
> >
> > This patch addresses PR tree-optimization/38943 where gcc may optimize
> > away trapping instructions even when -fnon-call-exceptions is specified.
> > Interestingly this only affects the C compiler (when -fexceptions is
> > not
> > specified) as g++ (or -fexceptions) supports C++-style exception
> > handling, where -fnon-call-exceptions triggers the stmt_could_throw_p machinery.
> > Without -fexceptions, trapping instructions aren't always considered
> > visible side-effects.
>
> But -fnon-call-exceptions without -fexceptions doesn't make much sense, does it?  I see the testcase behaves correctly when -fexceptions is also specified.
>
> The call vanishes in DCE because stmt_could_throw_p starts with
>
> bool
> stmt_could_throw_p (function *fun, gimple *stmt) {
>   if (!flag_exceptions)
>     return false;
>
> the documentation of -fnon-call-exceptions says
>
> Generate code that allows trapping instructions to throw exceptions.
>
> so either the above check is wrong or -fnon-call-exceptions should imply -fexceptions (or we should diagnose missing -fexceptions)
>
> >
> > This patch fixes this in two place.  Firstly, gimple_has_side_effects
> > is tweaked such that gimple_could_trap_p is considered a side-effect
> > if the current function can throw non-call exceptions.
>
> But exceptions are not considered side-effects - they are explicit in the IL and thus passes are supposed to check for those and preserve dead (externally) throwing stmts if not told otherwise (flag_delete_dead_exceptions).
>
> >  And secondly,
> > check_stmt in ipa-pure-const.c is tweaked such that a function
> > containing a trapping statement is considered to have a side-effect
> > with -fnon-call-exceptions, and therefore cannot be pure or const.
>
> EH is orthogonal to pure/const, so I think that's wrong.
>
> > Calling gimple_could_trap_p (which previously took a non-const gimple)
> > from gimple_has_side_effects (which takes a const gimple) required
> > improving the const-safety of gimple_could_trap_p (a relatively minor
> > tweak) and its prototypes.  Hopefully this is considered a clean-up/
> > improvement.
>
> Yeah, even an obvious one I think - you can push that independently.
>
> > This patch has been tested on x86_64-pc-linux-gnu with a "make
> > bootstrap" and "make -k check" with no new failures.  This should be
> > relatively safe, as there are no changes in behaviour unless the user
> > explicitly specifies -fnon-call-exceptions, when the C compiler then
> > behaves more like the C++/Ada compiler.
> >
> > Ok for mainline?
> >
> >
> > 2021-07-08  Roger Sayle  <roger@nextmovesoftware.com>
> >
> > gcc/ChangeLog
> >         PR tree-optimization/38943
> >         * gimple.c (gimple_has_side_effects): Consider trapping to
> >         be a side-effect when -fnon-call-exceptions is specified.
> >         (gimple_coult_trap_p_1):  Make S argument a "const gimple*".
> >         Preserve constness in call to gimple_asm_volatile_p.
> >         (gimple_could_trap_p): Make S argument a "const gimple*".
> >         * gimple.h (gimple_could_trap_p_1, gimple_could_trap_p):
> >         Update function prototypes.
> >         * ipa-pure-const.c (check_stmt): When the current function
> >         can throw non-call exceptions, a trapping statement should
> >         be considered a side-effect, so the function is neither
> >         const nor pure.
> >
> > gcc/testsuite/ChangeLog
> >         PR tree-optimization/38943
> >         * gcc.dg/pr38943.c: New test case.
> >
> > Roger
> > --
> > Roger Sayle
> > NextMove Software
> > Cambridge, UK
> >
Eric Botcazou July 12, 2021, 12:22 p.m. UTC | #2 
> There's still the open question what -fnon-call-exceptions on its
> own should do - IMHO it doesn't make sense to allow unwiding
> from a trapping memory reference but not from the call it resides
> in which means -fnon-call-exceptions should better enable
> -fexceptions?

Or issue a warning that it requires -fexceptions if the latter is not enabled?
Richard Biener July 12, 2021, 12:29 p.m. UTC | #3 
On Mon, Jul 12, 2021 at 2:22 PM Eric Botcazou <botcazou@adacore.com> wrote:
>
> > There's still the open question what -fnon-call-exceptions on its
> > own should do - IMHO it doesn't make sense to allow unwiding
> > from a trapping memory reference but not from the call it resides
> > in which means -fnon-call-exceptions should better enable
> > -fexceptions?
>
> Or issue a warning that it requires -fexceptions if the latter is not enabled?

Maybe that as well - I'd just like to avoid having the "undefined" state
flag_non_call_exceptions && ! flag_exceptions in the middle-end.

Well, unless somebody comes up with a good convincing use.

Richard.

> --
> Eric Botcazou
>
>
diff mbox series

Patch

diff --git a/gcc/common.opt b/gcc/common.opt
index a1353e0..7988653 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -2208,6 +2208,10 @@  fprefetch-loop-arrays
 Common Var(flag_prefetch_loop_arrays) Init(-1) Optimization
 Generate prefetch instructions, if available, for arrays in loops.
 
+fpreserve-traps
+Common Var(flag_preserve_traps)
+Treat trapping instructions as visible side-effects.
+
 fprofile
 Common Var(profile_flag)
 Enable basic program profiling code.
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index af2ce18..277eef5 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -655,7 +655,7 @@  Objective-C and Objective-C++ Dialects}.
 @gccoptlist{-fcall-saved-@var{reg}  -fcall-used-@var{reg} @gol
 -ffixed-@var{reg}  -fexceptions @gol
 -fnon-call-exceptions  -fdelete-dead-exceptions  -funwind-tables @gol
--fasynchronous-unwind-tables @gol
+-fasynchronous-unwind-tables -fpreserve-traps @gol
 -fno-gnu-unique @gol
 -finhibit-size-directive  -fcommon  -fno-ident @gol
 -fpcc-struct-return  -fpic  -fPIC  -fpie  -fPIE  -fno-plt @gol
@@ -16262,6 +16262,17 @@  Generate unwind table in DWARF format, if supported by target machine.  The
 table is exact at each instruction boundary, so it can be used for stack
 unwinding from asynchronous events (such as debugger or garbage collector).
 
+@item -fpreserve-traps
+@opindex fpreserve-traps
+Prevent trapping instructions that don't otherwise contribute to the
+execution of a program, from being optimized away.  This option is
+orthogonal to exception handling, though the @option{-fnon-call-exceptions}
+option may be used to throw exceptions from a trapping instruction,
+if supported by the target.  The set of instructions considered trapping
+may be controlled with @option{-ftrapping-math} and @option{-ftrapv}.
+This option treats traps as observable side-effects, hence a trapping
+instruction prevents a function from being @code{pure} or @code{const}.
+
 @item -fno-gnu-unique
 @opindex fno-gnu-unique
 @opindex fgnu-unique
diff --git a/gcc/gimple.c b/gcc/gimple.c
index f1044e9..66edc1e 100644
--- a/gcc/gimple.c
+++ b/gcc/gimple.c
@@ -2090,7 +2090,8 @@  gimple_move_vops (gimple *new_stmt, gimple *old_stmt)
    statement to have side effects if:
 
    - It is a GIMPLE_CALL not marked with ECF_PURE or ECF_CONST.
-   - Any of its operands are marked TREE_THIS_VOLATILE or TREE_SIDE_EFFECTS.  */
+   - Any of its operands are marked TREE_THIS_VOLATILE or TREE_SIDE_EFFECTS.
+   - It may trap and -fpreserve-traps has been specified.  */
 
 bool
 gimple_has_side_effects (const gimple *s)
@@ -2108,6 +2109,12 @@  gimple_has_side_effects (const gimple *s)
       && gimple_asm_volatile_p (as_a <const gasm *> (s)))
     return true;
 
+  /* Treat trapping instructions as having side-effects when
+     -fpreserve_traps has been specified.  */
+  if (flag_preserve_traps
+      && gimple_could_trap_p (s))
+    return true;
+
   if (is_gimple_call (s))
     {
       int flags = gimple_call_flags (s);
@@ -2129,7 +2136,7 @@  gimple_has_side_effects (const gimple *s)
    S is a GIMPLE_ASSIGN, the LHS of the assignment is also checked.  */
 
 bool
-gimple_could_trap_p_1 (gimple *s, bool include_mem, bool include_stores)
+gimple_could_trap_p_1 (const gimple *s, bool include_mem, bool include_stores)
 {
   tree t, div = NULL_TREE;
   enum tree_code op;
@@ -2146,7 +2153,7 @@  gimple_could_trap_p_1 (gimple *s, bool include_mem, bool include_stores)
   switch (gimple_code (s))
     {
     case GIMPLE_ASM:
-      return gimple_asm_volatile_p (as_a <gasm *> (s));
+      return gimple_asm_volatile_p (as_a <const gasm *> (s));
 
     case GIMPLE_CALL:
       t = gimple_call_fndecl (s);
@@ -2192,7 +2199,7 @@  gimple_could_trap_p_1 (gimple *s, bool include_mem, bool include_stores)
 /* Return true if statement S can trap.  */
 
 bool
-gimple_could_trap_p (gimple *s)
+gimple_could_trap_p (const gimple *s)
 {
   return gimple_could_trap_p_1 (s, true, true);
 }
diff --git a/gcc/gimple.h b/gcc/gimple.h
index e7dc2a4..1a2e120 100644
--- a/gcc/gimple.h
+++ b/gcc/gimple.h
@@ -1601,8 +1601,8 @@  void gimple_set_lhs (gimple *, tree);
 gimple *gimple_copy (gimple *);
 void gimple_move_vops (gimple *, gimple *);
 bool gimple_has_side_effects (const gimple *);
-bool gimple_could_trap_p_1 (gimple *, bool, bool);
-bool gimple_could_trap_p (gimple *);
+bool gimple_could_trap_p_1 (const gimple *, bool, bool);
+bool gimple_could_trap_p (const gimple *);
 bool gimple_assign_rhs_could_trap_p (gimple *);
 extern void dump_gimple_statistics (void);
 unsigned get_gimple_rhs_num_ops (enum tree_code);
diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c
index f045108..e2a4bcf 100644
--- a/gcc/ipa-pure-const.c
+++ b/gcc/ipa-pure-const.c
@@ -765,6 +765,14 @@  check_stmt (gimple_stmt_iterator *gsip, funct_state local, bool ipa)
       if (dump_file)
 	fprintf (dump_file, "    Volatile stmt is not const/pure\n");
     }
+  else if (flag_preserve_traps
+	   && gimple_code (stmt) != GIMPLE_CALL
+	   && gimple_could_trap_p (stmt))
+    {
+      local->pure_const_state = IPA_NEITHER;
+      if (dump_file)
+	fprintf (dump_file, "    Trapping stmt is not const/pure\n");
+    }
 
   /* Look for loads and stores.  */
   walk_stmt_load_store_ops (stmt, local,