Re: [GCC][PATCH] Enable underflow check in canonicalize_comparison. (PR86995)

[Vlad Lazar]

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.la...@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.la...@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.edlin...@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;