Re: [PATCH] loongarch: use -mno-check-zero-division as the default for optimized code

[Richard Biener <richard.guenther@gmail.com>]

On Thu, Jun 30, 2022 at 5:00 AM Xi Ruoyao via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi,
>
> We've made a consensus [1] that not to enable trapping for division by
> zero by default for LLVM, and we think GCC should behave similarly.
>
> The main rationales:
>
> 1. Division by zero is undefined behavior, so in theory any portable
> program shall not depend on it.
> 2. There are already many targets where both the hardware and GCC port
> do nothing to trap on division by zero.  A list taken from
> gcc.c-torture/execute/20101011-1.c:  PowerPC, RISC-V, ARM64, MSP430, and
> many others.  So in practice any portable program cannot depend on this
> trap.
> 3. As an ICPC assistant coach, I'm well aware that the main disadvantage
> not to trap on division by zero is "it breaks expectations of newbies".
> So, we keep -mcheck-zero-division defaulted for -O0 and -Og.  For other
> optimization levels, it's well known that UBs are already breaking
> newbies' expectations [2].
> 4. GCC is going to optimize more heavily exploiting integer division by
> zero [3].  So let's stop encouraging people to rely on any integer
> division by zero behavior from now.
>
> [1]: https://reviews.llvm.org/D128572/new/#3612039
> [2]: http://blog.llvm.org/2011/05/what-every-c-programmer-should-know_14.html
> [3]: https://gcc.gnu.org/pipermail/gcc-patches/2022-May/595099.html
>
> Patch content following.  Bootstrapped and regtested on loongarch64-
> linux-gnu. Ok for trunk?

It might be worth backporting this behavioral change to the GCC 12 branch
as well (so 12.1 is the only release with different default behavior) and
documenting the change in changes.html

> -- >8 --
>
> Integer division by zero is undefined behavior anyway, and there are
> already many platforms where neither the GCC port and the hardware do
> anything to trap on division by zero.  So any portable program shall not
> rely on SIGFPE on division by zero, in both theory and practice.  As the
> result, there is no real reason to cost two additional instructions just
> for the trap on division by zero with a new ISA.
>
> One remaining reason to trap on division by zero may be debugging,
> especially while -fsanitize=integer-divide-by-zero is not implemented
> for LoongArch yet.  To make debugging easier, keep -mcheck-zero-division
> as the default for -O0 and -Og, but use -mno-check-zero-division as the
> default for all other optimization levels.
>
> gcc/ChangeLog:
>
>         * config/loongarch/loongarch.cc (loongarch_check_zero_div_p):
>         New static function.
>         (loongarch_idiv_insns): Use loongarch_check_zero_div_p instead
>         of TARGET_CHECK_ZERO_DIV.
>         (loongarch_output_division): Likewise.
>         * doc/invoke.texi: Update to match the new behavior.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.c-torture/execute/20101011-1.c (dg-additional-options):
>         add -mcheck-zero-division for LoongArch targets.
> ---
>  gcc/config/loongarch/loongarch.cc              | 18 +++++++++++++++---
>  gcc/doc/invoke.texi                            |  3 ++-
>  .../gcc.c-torture/execute/20101011-1.c         |  1 +
>  3 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/gcc/config/loongarch/loongarch.cc b/gcc/config/loongarch/loongarch.cc
> index c8502b0b0f3..f297083c2e9 100644
> --- a/gcc/config/loongarch/loongarch.cc
> +++ b/gcc/config/loongarch/loongarch.cc
> @@ -2101,6 +2101,19 @@ loongarch_load_store_insns (rtx mem, rtx_insn *insn)
>    return loongarch_address_insns (XEXP (mem, 0), mode, might_split_p);
>  }
>
> +/* Return true if we need to trap on division by zero.  */
> +
> +static bool
> +loongarch_check_zero_div_p (void)
> +{
> +  /* if -m[no-]check-zero-division is given explicitly.  */
> +  if (target_flags_explicit & MASK_CHECK_ZERO_DIV)
> +    return TARGET_CHECK_ZERO_DIV;
> +
> +  /* if not, don't trap for optimized code except -Og.  */
> +  return !optimize || optimize_debug;
> +}
> +
>  /* Return the number of instructions needed for an integer division.  */
>
>  int
> @@ -2109,7 +2122,7 @@ loongarch_idiv_insns (machine_mode mode ATTRIBUTE_UNUSED)
>    int count;
>
>    count = 1;
> -  if (TARGET_CHECK_ZERO_DIV)
> +  if (loongarch_check_zero_div_p ())
>      count += 2;
>
>    return count;
> @@ -4050,7 +4063,6 @@ loongarch_do_optimize_block_move_p (void)
>    return !optimize_size;
>  }
>
> -
>  /* Expand a QI or HI mode atomic memory operation.
>
>     GENERATOR contains a pointer to the gen_* function that generates
> @@ -5262,7 +5274,7 @@ loongarch_output_division (const char *division, rtx *operands)
>    const char *s;
>
>    s = division;
> -  if (TARGET_CHECK_ZERO_DIV)
> +  if (loongarch_check_zero_div_p ())
>      {
>        output_asm_insn (s, operands);
>        s = "bne\t%2,%.,1f\n\tbreak\t7\n1:";
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index bde59ff0472..7e2a0b01233 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -24740,7 +24740,8 @@ Set the cost of branches to roughly @var{n} instructions.
>  @itemx -mno-check-zero-divison
>  @opindex -mcheck-zero-division
>  Trap (do not trap) on integer division by zero.  The default is
> -@option{-mcheck-zero-division}.
> +@option{-mcheck-zero-division} for @option{-O0} or @option{-Og}, and
> +@option{-mno-check-zero-division} for other optimization levels.
>
>  @item -mcond-move-int
>  @itemx -mno-cond-move-int
> diff --git a/gcc/testsuite/gcc.c-torture/execute/20101011-1.c b/gcc/testsuite/gcc.c-torture/execute/20101011-1.c
> index 649e168e0b1..d2c0f9ab7ec 100644
> --- a/gcc/testsuite/gcc.c-torture/execute/20101011-1.c
> +++ b/gcc/testsuite/gcc.c-torture/execute/20101011-1.c
> @@ -1,6 +1,7 @@
>  /* { dg-options "-fnon-call-exceptions" } */
>  /* With -fnon-call-exceptions 0 / 0 should not be eliminated.  */
>  /* { dg-additional-options "-DSIGNAL_SUPPRESS" { target { ! signal } } } */
> +/* { dg-additional-options "-mcheck-zero-division" { target { loongarch*-*-* } } } */
>
>  #ifdef SIGNAL_SUPPRESS
>  # define DO_TEST 0
> --
> 2.36.0
>
>