diff mbox series

Enable underflow check in canonicalize_comparison. (PR86995)

Message ID 5B86CC3E.40006@arm.com
State New
Headers show
Series Enable underflow check in canonicalize_comparison. (PR86995) | expand

Commit Message

Vlad Lazar Aug. 29, 2018, 4:39 p.m. UTC 
Hi.

r263591 introduced the following regressions on multiple platforms:
+FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O0  execution test
+FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O2  execution test
+FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O2 -flto  execution test
+FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O2 -flto -flto-partition=none  execution test
+FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c   -O0  execution test
+FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c   -O2  execution test
+FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c   -O2 -flto  execution test
+FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c   -O2 -flto -flto-partition=none  execution test

The cause for this was that in canonicalize_comparison wi::add was used to add a negative number.  This meant
that in case of underflow the flag would not have been set.  The solution is to use wi::sub if the immediate
needs to be decremented.

The patch fixes the mentioned regressions.
Bootstrapped and regtested on aarch64-none-linux-gnu and x86_64-pc-linux-gnu.

Thanks,
Vlad

---

Comments

Jakub Jelinek Aug. 29, 2018, 4:43 p.m. UTC | #1 
On Wed, Aug 29, 2018 at 05:39:26PM +0100, Vlad Lazar wrote:
> r263591 introduced the following regressions on multiple platforms:
> +FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O0  execution test
> +FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O2  execution test
> +FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O2 -flto  execution test
> +FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O2 -flto -flto-partition=none  execution test
> +FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c   -O0  execution test
> +FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c   -O2  execution test
> +FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c   -O2 -flto  execution test
> +FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c   -O2 -flto -flto-partition=none  execution test
> 
> The cause for this was that in canonicalize_comparison wi::add was used to add a negative number.  This meant
> that in case of underflow the flag would not have been set.  The solution is to use wi::sub if the immediate
> needs to be decremented.
> 
> The patch fixes the mentioned regressions.
> Bootstrapped and regtested on aarch64-none-linux-gnu and x86_64-pc-linux-gnu.

LGTM, but ChangeLog entry is missing, can you please provide one before I
can ack it?

> diff --git a/gcc/expmed.c b/gcc/expmed.c
> index be9f0ec9011..84f58f540ab 100644
> --- a/gcc/expmed.c
> +++ b/gcc/expmed.c
> @@ -6235,7 +6235,13 @@ canonicalize_comparison (machine_mode mode, enum rtx_code *code, rtx *imm)
>       wrapping around in the case of unsigned values.  If any occur
>       cancel the optimization.  */
>    wi::overflow_type overflow = wi::OVF_NONE;
> -  wide_int imm_modif = wi::add (imm_val, to_add, sgn, &overflow);
> +  wide_int imm_modif;
> +
> +  if (to_add == 1)
> +    imm_modif = wi::add (imm_val, 1, sgn, &overflow);
> +  else
> +    imm_modif = wi::sub (imm_val, 1, sgn, &overflow);
> +
>    if (overflow)
>      return;

	Jakub
Vlad Lazar Aug. 29, 2018, 4:49 p.m. UTC | #2 
On 29/08/18 17:43, Jakub Jelinek wrote:
> On Wed, Aug 29, 2018 at 05:39:26PM +0100, Vlad Lazar wrote:
>> r263591 introduced the following regressions on multiple platforms:
>> +FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O0  execution test
>> +FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O2  execution test
>> +FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O2 -flto  execution test
>> +FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O2 -flto -flto-partition=none  execution test
>> +FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c   -O0  execution test
>> +FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c   -O2  execution test
>> +FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c   -O2 -flto  execution test
>> +FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c   -O2 -flto -flto-partition=none  execution test
>>
>> The cause for this was that in canonicalize_comparison wi::add was used to add a negative number.  This meant
>> that in case of underflow the flag would not have been set.  The solution is to use wi::sub if the immediate
>> needs to be decremented.
>>
>> The patch fixes the mentioned regressions.
>> Bootstrapped and regtested on aarch64-none-linux-gnu and x86_64-pc-linux-gnu.
>
> LGTM, but ChangeLog entry is missing, can you please provide one before I
> can ack it?
>
Sorry about that. Here's the ChangeLog:

gcc/
2018-08-29  Vlad Lazar  <vlad.lazar@arm.com>
	* expmed.c (canonicalize_comparison): Enable underflow check.

>> diff --git a/gcc/expmed.c b/gcc/expmed.c
>> index be9f0ec9011..84f58f540ab 100644
>> --- a/gcc/expmed.c
>> +++ b/gcc/expmed.c
>> @@ -6235,7 +6235,13 @@ canonicalize_comparison (machine_mode mode, enum rtx_code *code, rtx *imm)
>>        wrapping around in the case of unsigned values.  If any occur
>>        cancel the optimization.  */
>>     wi::overflow_type overflow = wi::OVF_NONE;
>> -  wide_int imm_modif = wi::add (imm_val, to_add, sgn, &overflow);
>> +  wide_int imm_modif;
>> +
>> +  if (to_add == 1)
>> +    imm_modif = wi::add (imm_val, 1, sgn, &overflow);
>> +  else
>> +    imm_modif = wi::sub (imm_val, 1, sgn, &overflow);
>> +
>>     if (overflow)
>>       return;
>
> 	Jakub
>
Jakub Jelinek Aug. 29, 2018, 4:55 p.m. UTC | #3 
On Wed, Aug 29, 2018 at 05:49:02PM +0100, Vlad Lazar wrote:
> On 29/08/18 17:43, Jakub Jelinek wrote:
> > On Wed, Aug 29, 2018 at 05:39:26PM +0100, Vlad Lazar wrote:
> > > r263591 introduced the following regressions on multiple platforms:
> > > +FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O0  execution test
> > > +FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O2  execution test
> > > +FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O2 -flto  execution test
> > > +FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O2 -flto -flto-partition=none  execution test
> > > +FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c   -O0  execution test
> > > +FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c   -O2  execution test
> > > +FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c   -O2 -flto  execution test
> > > +FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c   -O2 -flto -flto-partition=none  execution test
> > > 
> > > The cause for this was that in canonicalize_comparison wi::add was used to add a negative number.  This meant
> > > that in case of underflow the flag would not have been set.  The solution is to use wi::sub if the immediate
> > > needs to be decremented.
> > > 
> > > The patch fixes the mentioned regressions.
> > > Bootstrapped and regtested on aarch64-none-linux-gnu and x86_64-pc-linux-gnu.
> > 
> > LGTM, but ChangeLog entry is missing, can you please provide one before I
> > can ack it?
> > 
> Sorry about that. Here's the ChangeLog:
> 
> gcc/
> 2018-08-29  Vlad Lazar  <vlad.lazar@arm.com>
> 	* expmed.c (canonicalize_comparison): Enable underflow check.

There should be empty line after the date/name/email line.
For ChangeLog entries of GCC bugzilla PR bugfixes then the next line
should be
	PR middle-end/86995
and only then the expmed.c line.  "Enable underflow check." doesn't look
sufficiently descriptive to what it does, I'd use
	* expmed.c (canonicalize_comparison): Use wi::sub instead of wi::add
	if to_add is negative.

Ok for trunk with those changes.

	Jakub
Vlad Lazar Aug. 30, 2018, 9:34 a.m. UTC | #4 
On 29/08/18 17:55, Jakub Jelinek wrote:
> On Wed, Aug 29, 2018 at 05:49:02PM +0100, Vlad Lazar wrote:
>> On 29/08/18 17:43, Jakub Jelinek wrote:
>>> On Wed, Aug 29, 2018 at 05:39:26PM +0100, Vlad Lazar wrote:
>>>> r263591 introduced the following regressions on multiple platforms:
>>>> +FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O0  execution test
>>>> +FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O2  execution test
>>>> +FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O2 -flto  execution test
>>>> +FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O2 -flto -flto-partition=none  execution test
>>>> +FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c   -O0  execution test
>>>> +FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c   -O2  execution test
>>>> +FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c   -O2 -flto  execution test
>>>> +FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c   -O2 -flto -flto-partition=none  execution test
>>>>
>>>> The cause for this was that in canonicalize_comparison wi::add was used to add a negative number.  This meant
>>>> that in case of underflow the flag would not have been set.  The solution is to use wi::sub if the immediate
>>>> needs to be decremented.
>>>>
>>>> The patch fixes the mentioned regressions.
>>>> Bootstrapped and regtested on aarch64-none-linux-gnu and x86_64-pc-linux-gnu.
>>>
>>> LGTM, but ChangeLog entry is missing, can you please provide one before I
>>> can ack it?
>>>
>> Sorry about that. Here's the ChangeLog:
>>
>> gcc/
>> 2018-08-29  Vlad Lazar  <vlad.lazar@arm.com>
>> 	* expmed.c (canonicalize_comparison): Enable underflow check.
>
> There should be empty line after the date/name/email line.
> For ChangeLog entries of GCC bugzilla PR bugfixes then the next line
> should be
> 	PR middle-end/86995
> and only then the expmed.c line.  "Enable underflow check." doesn't look
> sufficiently descriptive to what it does, I'd use
> 	* expmed.c (canonicalize_comparison): Use wi::sub instead of wi::add
> 	if to_add is negative.
>
> Ok for trunk with those changes.
>
> 	Jakub
>
Thanks for the swift review.
I've attached the committed patch.

Thanks,
Vlad
Index: ChangeLog
===================================================================
--- ChangeLog	(revision 263972)
+++ ChangeLog	(working copy)
@@ -1,3 +1,9 @@
+2018-08-30  Vlad Lazar  <vlad.lazar@arm.com>
+
+	PR middle-end/86995
+	* expmed.c (canonicalize_comparison): Use wi::sub instead of wi::add
+	if to_add is negative.
+
 2018-08-29  Bernd Edlinger  <bernd.edlinger@hotmail.de>
 
 	PR middle-end/87053
Index: expmed.c
===================================================================
--- expmed.c	(revision 263972)
+++ expmed.c	(working copy)
@@ -6239,7 +6239,13 @@
      wrapping around in the case of unsigned values.  If any occur
      cancel the optimization.  */
   wi::overflow_type overflow = wi::OVF_NONE;
-  wide_int imm_modif = wi::add (imm_val, to_add, sgn, &overflow);
+  wide_int imm_modif;
+
+  if (to_add == 1)
+    imm_modif = wi::add (imm_val, 1, sgn, &overflow);
+  else
+    imm_modif = wi::sub (imm_val, 1, sgn, &overflow);
+
   if (overflow)
     return;
diff mbox series

Patch

diff --git a/gcc/expmed.c b/gcc/expmed.c
index be9f0ec9011..84f58f540ab 100644
--- a/gcc/expmed.c
+++ b/gcc/expmed.c
@@ -6235,7 +6235,13 @@  canonicalize_comparison (machine_mode mode, enum rtx_code *code, rtx *imm)
       wrapping around in the case of unsigned values.  If any occur
       cancel the optimization.  */
    wi::overflow_type overflow = wi::OVF_NONE;
-  wide_int imm_modif = wi::add (imm_val, to_add, sgn, &overflow);
+  wide_int imm_modif;
+
+  if (to_add == 1)
+    imm_modif = wi::add (imm_val, 1, sgn, &overflow);
+  else
+    imm_modif = wi::sub (imm_val, 1, sgn, &overflow);
+
    if (overflow)
      return;