This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix divmod expansion (PR middle-end/78416)
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Jakub Jelinek <jakub at redhat dot com>,Richard Biener <rguenther at suse dot de>,Jeff Law <law at redhat dot com>
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Fri, 18 Nov 2016 23:10:58 +0100
- Subject: Re: [PATCH] Fix divmod expansion (PR middle-end/78416)
- Authentication-results: sourceware.org; auth=none
- References: <20161118214510.GM3541@tucnak.redhat.com>
On November 18, 2016 10:45:10 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote:
>Hi!
>
>As the testcase shows, expand_divmod doesn't handle properly
>division/remainder in modes larger than HOST_BITS_PER_WIDE_INT like
>TImode - if op1 is "negative" CONST_INT, it means it has 65 most
>significant
>bits set, but EXACT_POWER_OF_2_OR_ZERO_P will happily return true on
>that
>and optimize the division into a right shift.
>Fixed by taking into account that EXACT_POWER_OF_2_OR_ZERO_P works only
>if either the mode bitsize/precision are smaller or equal than HWI, or
>if the value doesn't have MSB bit set.
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
I wonder if transforming the const-int to wide int makes this all easier to read?
>2016-11-18 Jakub Jelinek <jakub@redhat.com>
>
> PR middle-end/78416
> * expmed.c (expand_divmod): For modes wider than HWI, take into
> account implicit 1 bits above msb for EXACT_POWER_OF_2_OR_ZERO_P.
> Formatting fixes. Use size <= HOST_BITS_PER_WIDE_INT instead of
> HOST_BITS_PER_WIDE_INT >= size.
>
> * gcc.dg/torture/pr78416.c: New test.
>
>--- gcc/expmed.c.jj 2016-10-31 13:28:12.000000000 +0100
>+++ gcc/expmed.c 2016-11-18 16:57:57.608703605 +0100
>@@ -3998,7 +3998,15 @@ expand_divmod (int rem_flag, enum tree_c
> if (unsignedp)
> ext_op1 &= GET_MODE_MASK (mode);
> op1_is_pow2 = ((EXACT_POWER_OF_2_OR_ZERO_P (ext_op1)
>- || (! unsignedp && EXACT_POWER_OF_2_OR_ZERO_P (-ext_op1))));
>+ /* If mode is wider than HWI and op1 has msb set,
>+ then it has there extra implicit 1 bits above it. */
>+ && (GET_MODE_PRECISION (mode) <= HOST_BITS_PER_WIDE_INT
>+ || INTVAL (op1) >= 0))
>+ || (! unsignedp
>+ && EXACT_POWER_OF_2_OR_ZERO_P (-ext_op1)
>+ && (GET_MODE_PRECISION (mode)
>+ <= HOST_BITS_PER_WIDE_INT
>+ || INTVAL (op1) < 0)));
> }
>
> /*
>@@ -4141,8 +4149,17 @@ expand_divmod (int rem_flag, enum tree_c
> op1_is_constant = CONST_INT_P (op1);
> op1_is_pow2 = (op1_is_constant
> && ((EXACT_POWER_OF_2_OR_ZERO_P (INTVAL (op1))
>- || (! unsignedp
>- && EXACT_POWER_OF_2_OR_ZERO_P (-UINTVAL (op1))))));
>+ /* If mode is wider than HWI and op1 has msb set,
>+ then it has there extra implicit 1 bits above
>+ it. */
>+ && (GET_MODE_PRECISION (compute_mode)
>+ <= HOST_BITS_PER_WIDE_INT
>+ || INTVAL (op1) >= 0))
>+ || (! unsignedp
>+ && EXACT_POWER_OF_2_OR_ZERO_P (-UINTVAL (op1))
>+ && (GET_MODE_PRECISION (compute_mode)
>+ <= HOST_BITS_PER_WIDE_INT
>+ || INTVAL (op1) < 0))));
> }
>
>/* If one of the operands is a volatile MEM, copy it into a register.
>*/
>@@ -4185,7 +4202,8 @@ expand_divmod (int rem_flag, enum tree_c
> unsigned HOST_WIDE_INT d = (INTVAL (op1)
> & GET_MODE_MASK (compute_mode));
>
>- if (EXACT_POWER_OF_2_OR_ZERO_P (d))
>+ if (EXACT_POWER_OF_2_OR_ZERO_P (d)
>+ && (INTVAL (op1) >= 0 || size <= HOST_BITS_PER_WIDE_INT))
> {
> pre_shift = floor_log2 (d);
> if (rem_flag)
>@@ -4325,7 +4343,7 @@ expand_divmod (int rem_flag, enum tree_c
> else if (d == -1)
> quotient = expand_unop (compute_mode, neg_optab, op0,
> tquotient, 0);
>- else if (HOST_BITS_PER_WIDE_INT >= size
>+ else if (size <= HOST_BITS_PER_WIDE_INT
> && abs_d == HOST_WIDE_INT_1U << (size - 1))
> {
> /* This case is not handled correctly below. */
>@@ -4335,6 +4353,7 @@ expand_divmod (int rem_flag, enum tree_c
> goto fail1;
> }
> else if (EXACT_POWER_OF_2_OR_ZERO_P (d)
>+ && (size <= HOST_BITS_PER_WIDE_INT || d >= 0)
> && (rem_flag
> ? smod_pow2_cheap (speed, compute_mode)
> : sdiv_pow2_cheap (speed, compute_mode))
>@@ -4348,7 +4367,9 @@ expand_divmod (int rem_flag, enum tree_c
> compute_mode)
> != CODE_FOR_nothing)))
> ;
>- else if (EXACT_POWER_OF_2_OR_ZERO_P (abs_d))
>+ else if (EXACT_POWER_OF_2_OR_ZERO_P (abs_d)
>+ && (size <= HOST_BITS_PER_WIDE_INT
>+ || abs_d != (unsigned HOST_WIDE_INT) d))
> {
> if (rem_flag)
> {
>@@ -4483,7 +4504,7 @@ expand_divmod (int rem_flag, enum tree_c
> case FLOOR_DIV_EXPR:
> case FLOOR_MOD_EXPR:
> /* We will come here only for signed operations. */
>- if (op1_is_constant && HOST_BITS_PER_WIDE_INT >= size)
>+ if (op1_is_constant && size <= HOST_BITS_PER_WIDE_INT)
> {
> unsigned HOST_WIDE_INT mh, ml;
> int pre_shift, lgup, post_shift;
>@@ -4552,9 +4573,8 @@ expand_divmod (int rem_flag, enum tree_c
> op0, constm1_rtx), NULL_RTX);
> t2 = expand_binop (compute_mode, ior_optab, op0, t1, NULL_RTX,
> 0, OPTAB_WIDEN);
>- nsign = expand_shift
>- (RSHIFT_EXPR, compute_mode, t2,
>- size - 1, NULL_RTX, 0);
>+ nsign = expand_shift (RSHIFT_EXPR, compute_mode, t2,
>+ size - 1, NULL_RTX, 0);
> t3 = force_operand (gen_rtx_MINUS (compute_mode, t1, nsign),
> NULL_RTX);
> t4 = expand_divmod (0, TRUNC_DIV_EXPR, compute_mode, t3, op1,
>@@ -4663,7 +4683,10 @@ expand_divmod (int rem_flag, enum tree_c
> case CEIL_MOD_EXPR:
> if (unsignedp)
> {
>- if (op1_is_constant && EXACT_POWER_OF_2_OR_ZERO_P (INTVAL (op1)))
>+ if (op1_is_constant
>+ && EXACT_POWER_OF_2_OR_ZERO_P (INTVAL (op1))
>+ && (size <= HOST_BITS_PER_WIDE_INT
>+ || INTVAL (op1) >= 0))
> {
> rtx t1, t2, t3;
> unsigned HOST_WIDE_INT d = INTVAL (op1);
>@@ -4876,7 +4899,7 @@ expand_divmod (int rem_flag, enum tree_c
> break;
>
> case EXACT_DIV_EXPR:
>- if (op1_is_constant && HOST_BITS_PER_WIDE_INT >= size)
>+ if (op1_is_constant && size <= HOST_BITS_PER_WIDE_INT)
> {
> HOST_WIDE_INT d = INTVAL (op1);
> unsigned HOST_WIDE_INT ml;
>--- gcc/testsuite/gcc.dg/torture/pr78416.c.jj 2016-11-18
>16:43:35.010448325 +0100
>+++ gcc/testsuite/gcc.dg/torture/pr78416.c 2016-11-18
>16:44:40.388634204 +0100
>@@ -0,0 +1,17 @@
>+/* PR middle-end/78416 */
>+/* { dg-do run { target int128 } } */
>+
>+int
>+main ()
>+{
>+ unsigned __int128 x;
>+ x = 0xFFFFFFFFFFFFFFFFULL;
>+ x /= ~0x7FFFFFFFFFFFFFFFLL;
>+ if (x != 0)
>+ __builtin_abort ();
>+ x = ~0x7FFFFFFFFFFFFFFELL;
>+ x /= ~0x7FFFFFFFFFFFFFFFLL;
>+ if (x != 1)
>+ __builtin_abort ();
>+ return 0;
>+}
>
> Jakub