Bug 81162 - [8 Regression] UBSAN switch triggers incorrect optimization in SLSR
Summary: [8 Regression] UBSAN switch triggers incorrect optimization in SLSR
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 8.0
: P3 normal
Target Milestone: 8.0
Assignee: Bill Schmidt
URL:
Keywords: wrong-code
Depends on:
Blocks: yarpgen
  Show dependency treegraph
 
Reported: 2017-06-21 22:06 UTC by Dmitry Babokin
Modified: 2021-11-01 23:07 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work: 8.0
Known to fail:
Last reconfirmed: 2017-06-22 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Babokin 2017-06-21 22:06:45 UTC
gcc trunk rev249427, x86_64

In presence of -fsanitize=undefined optimisations (-O2) produce wrong code.

> cat f.cpp
#include <stdio.h>
short s;
int i1 = 1;
int i2 = 1;
unsigned char uc = 147;

int main() {
  s = (-uc + 2147483647) << 0;
  if (9031239389974324562ULL >= (-((i1 && i2) + uc) ^ -21096) ) {
    printf("OK\n");
  } else {
    printf("FAIL\n");
  }

  return 0;
}

> g++ -std=c++11 -fsanitize=undefined -O0 f.cpp -o out
> ./out
OK
> g++ -std=c++11 -fsanitize=undefined -O2 f.cpp -o out
> ./out
FAIL
Comment 1 Marek Polacek 2017-06-22 09:48:52 UTC
Confirmed, started with r247578.
Comment 2 Marek Polacek 2017-06-22 10:11:35 UTC
-fsanitize=integer-divide-by-zero is what seems to trigger this.
Comment 3 Marek Polacek 2017-06-22 10:26:08 UTC
And the difference is that with -fsanitize=integer-divide-by-zero we'll do this:
5257   if (sanitize_flags_p ((SANITIZE_SHIFT
5258                          | SANITIZE_DIVIDE | SANITIZE_FLOAT_DIVIDE))
5259       && !processing_template_decl 
5260       && (doing_div_or_mod || doing_shift))
5261     {
5262       /* OP0 and/or OP1 might have side-effects.  */
5263       op0 = cp_save_expr (op0);
5264       op1 = cp_save_expr (op1);

so in .original there's
(void) (s = (short int) (NON_LVALUE_EXPR <SAVE_EXPR <-(int) uc + 2147483647>>))
instead of
(void) (s = (short int) ~(unsigned short) uc)
Comment 4 Richard Biener 2017-06-22 10:51:37 UTC
I'll have a looksee.
Comment 5 Richard Biener 2017-06-29 07:52:53 UTC
So we go i1 && i2 -> FAIL.  After VRP1:

  # RANGE [0, 255] NONZERO 255
  _2 = (int) uc.0_1;
...
  if (i1 && i2)
  <bb 5> [100.00%] [count: INV]:
  # RANGE [0, 1]
  # iftmp.1_14 = PHI <1(3), 0(4)>
  # RANGE [0, 1] NONZERO 1
  _7 = (int) iftmp.1_14;
  # RANGE [0, 256] NONZERO 511
  _10 = _7 + _2;
  # RANGE [-256, 0]
  _11 = 0 - _10;
  _12 = _11 ^ -21096;
  # RANGE ~[2147483648, 18446744071562067967]
  _13 = (long long unsigned int) _12;
  if (_13 <= 9031239389974324562)
    OK
  else
    FAIL

the unfolded 0 - _10 gets cleaned up by forwprop3 to

  # RANGE [-256, 0]
  _11 = -_10;

PRE then takes the opportunity to optimize this to

  <bb 5> [100.00%] [count: INV]:
  # RANGE [0, 1]
  # iftmp.1_14 = PHI <1(4), 0(3), 0(2)>
  # RANGE [-256, 0]
  # prephitmp_27 = PHI <_26(4), _3(3), _3(2)>
  _12 = prephitmp_27 ^ -21096;
  # RANGE ~[2147483648, 18446744071562067967]
  _13 = (long long unsigned int) _12;
  if (_13 <= 9031239389974324562)

which still looks correct to me.  Then slsr seems to fold

  <bb 2> [100.00%] [count: INV]:
  uc.0_1 = uc;
  # RANGE [0, 255] NONZERO 255
  _2 = (int) uc.0_1;
  # RANGE [-255, 0]
  _3 = -_2;
  # RANGE [2147483392, 2147483647] NONZERO 2147483647
  _17 = _3 + 2147483647;
...
   <bb 4> [25.00%] [count: INV]:
   _24 = _2 + 1;
-  _26 = -_24;
+  _26 = _17 - -2147483648;

somehow which looks really odd... and from which VRP2 rightfully derives

_26: [2147483647, +INF]

Strength reduction candidate vector:

  1  [2] _3 = -_2;
     MULT : (_2 + 0) * -1 : int
     basis: 0  dependent: 2  sibling: 0
     next-interp: 0  dead-savings: 0

  2  [2] _17 = _3 + 2147483647;
     MULT : (_2 + -2147483647) * -1 : int
     basis: 1  dependent: 4  sibling: 0
     next-interp: 0  dead-savings: 0

I think this is an invalid candidate given undefined integer overflow semantics.
We have to be very careful here or emit all new expressions in unsigned
arithmetic.

  3  [4] _24 = _2 + 1;
     ADD  : _2 + (1 * 1) : int
     basis: 0  dependent: 0  sibling: 0
     next-interp: 0  dead-savings: 0

  4  [4] _26 = -_24;
     MULT : (_2 + 1) * -1 : int
     basis: 2  dependent: 0  sibling: 0
     next-interp: 0  dead-savings: 4

that looks ok but we seem to end up relating this to [2]:

Strength reduction candidate chains:

_2 -> 1 -> 4 -> 3 -> 2
_12 -> 5 -> 6


Processing dependency tree rooted at 1.
Replacing: _26 = -_24;
With: _26 = _17 - -2147483648;

cost-wise we see that _24 is eliminated by the transform I guess.

Bill?
Comment 6 Bill Schmidt 2017-06-29 14:26:35 UTC
Hm, thought we had all of these overflow cases sorted out long ago.  I'll have a look.  I'll be out on vacation shortly so this may not get fixed until second week of July if it's not immediately obvious.
Comment 7 Bill Schmidt 2017-06-30 16:55:51 UTC
This case comes up when we're going to replace a NEGATE_EXPR with a PLUS_EXPR or MINUS_EXPR.  This is another case of an unprofitable replacement that should be avoided anyway.  So I think the following fix is what we need:

Index: gcc/gimple-ssa-strength-reduction.c
===================================================================
--- gcc/gimple-ssa-strength-reduction.c	(revision 249846)
+++ gcc/gimple-ssa-strength-reduction.c	(working copy)
@@ -2082,13 +2082,14 @@ replace_mult_candidate (slsr_cand_t c, tree basis_
      types but allows for safe negation without twisted logic.  */
   if (wi::fits_shwi_p (bump)
       && bump.to_shwi () != HOST_WIDE_INT_MIN
-      /* It is not useful to replace casts, copies, or adds of
+      /* It is not useful to replace casts, copies, negates, or adds of
 	 an SSA name and a constant.  */
       && cand_code != SSA_NAME
       && !CONVERT_EXPR_CODE_P (cand_code)
       && cand_code != PLUS_EXPR
       && cand_code != POINTER_PLUS_EXPR
-      && cand_code != MINUS_EXPR)
+      && cand_code != MINUS_EXPR
+      && cand_code != NEGATE_EXPR)
     {
       enum tree_code code = PLUS_EXPR;
       tree bump_tree;

This fixes the particular test.  I have to head out on vacation now, but I will regstrap this and submit it when I get back if there are no objections.
Comment 8 Bill Schmidt 2017-07-14 18:07:17 UTC
Author: wschmidt
Date: Fri Jul 14 18:06:45 2017
New Revision: 250212

URL: https://gcc.gnu.org/viewcvs?rev=250212&root=gcc&view=rev
Log:
[gcc]

2016-07-14  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

	PR tree-optimization/81162
	* gimple-ssa-strength-reduction.c (replace_mult_candidate): Don't
	replace a negate with an add.

[gcc/testsuite]

2016-07-14  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

	PR tree-optimization/81162
	* gcc.dg/pr81162.c: New file.


Added:
    trunk/gcc/testsuite/gcc.dg/pr81162.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/gimple-ssa-strength-reduction.c
    trunk/gcc/testsuite/ChangeLog
Comment 9 Bill Schmidt 2017-07-14 18:08:53 UTC
Fixed in trunk so far.  Need to verify that backports are needed, but I suspect they are.
Comment 10 Bernd Edlinger 2017-07-17 11:48:53 UTC
The test case don't run if gcc-4.6 is host compiler,
this means it links against the host compiler libubsan.so.0
which is not really the correct thing to do:

Setting LD_LIBRARY_PATH to :/home/ed/gnu/gcc-build/gcc:/home/ed/gnu/gcc-build/gcc/32:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/./libatomic/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/./libcilkrts/.libs::/home/ed/gnu/gcc-build/gcc:/home/ed/gnu/gcc-build/gcc/32:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/./libatomic/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/./libcilkrts/.libs:/home/ed/gnu/gcc-build/./gmp/.libs:/home/ed/gnu/gcc-build/./prev-gmp/.libs:/home/ed/gnu/gcc-build/./mpfr/src/.libs:/home/ed/gnu/gcc-build/./prev-mpfr/src/.libs:/home/ed/gnu/gcc-build/./mpc/src/.libs:/home/ed/gnu/gcc-build/./prev-mpc/src/.libs:/home/ed/gnu/gcc-build/./isl/.libs:/home/ed/gnu/gcc-build/./prev-isl/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/src/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libsanitizer/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libmpx/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libvtv/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libcilkrts/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libssp/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libgomp/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libitm/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libatomic/.libs:/home/ed/gnu/gcc-build/./gcc:/home/ed/gnu/gcc-build/./prev-gcc:/home/ed/gnu/gcc-build/./gmp/.libs:/home/ed/gnu/gcc-build/./prev-gmp/.libs:/home/ed/gnu/gcc-build/./mpfr/src/.libs:/home/ed/gnu/gcc-build/./prev-mpfr/src/.libs:/home/ed/gnu/gcc-build/./mpc/src/.libs:/home/ed/gnu/gcc-build/./prev-mpc/src/.libs:/home/ed/gnu/gcc-build/./isl/.libs:/home/ed/gnu/gcc-build/./prev-isl/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/src/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libsanitizer/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libmpx/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libvtv/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libcilkrts/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libssp/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libgomp/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libitm/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libatomic/.libs:/home/ed/gnu/gcc-build/./gcc:/home/ed/gnu/gcc-build/./prev-gcc
spawn [open ...]^M
./pr81162.exe: error while loading shared libraries: libubsan.so.0: cannot open shared object file: No such file or directory
FAIL: gcc.dg/pr81162.c execution test
Comment 11 rguenther@suse.de 2017-07-17 12:33:11 UTC
On Mon, 17 Jul 2017, bernd.edlinger at hotmail dot de wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81162
> 
> Bernd Edlinger <bernd.edlinger at hotmail dot de> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                  CC|                            |bernd.edlinger at hotmail dot de
> 
> --- Comment #10 from Bernd Edlinger <bernd.edlinger at hotmail dot de> ---
> The test case don't run if gcc-4.6 is host compiler,
> this means it links against the host compiler libubsan.so.0
> which is not really the correct thing to do:
> 
> Setting LD_LIBRARY_PATH to
> :/home/ed/gnu/gcc-build/gcc:/home/ed/gnu/gcc-build/gcc/32:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/./libatomic/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/./libcilkrts/.libs::/home/ed/gnu/gcc-build/gcc:/home/ed/gnu/gcc-build/gcc/32:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/./libatomic/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/./libcilkrts/.libs:/home/ed/gnu/gcc-build/./gmp/.libs:/home/ed/gnu/gcc-build/./prev-gmp/.libs:/home/ed/gnu/gcc-build/./mpfr/src/.libs:/home/ed/gnu/gcc-build/./prev-mpfr/src/.libs:/home/ed/gnu/gcc-build/./mpc/src/.libs:/home/ed/gnu/gcc-build/./prev-mpc/src/.libs:/home/ed/gnu/gcc-build/./isl/.libs:/home/ed/gnu/gcc-build/./prev-isl/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/src/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libsanitizer/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libmpx/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libvtv/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libcilkrts/.libs:/home/ed/gnu/gcc
 -build/x86_64-pc-linux-gnu/libssp/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libgomp/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libitm/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libatomic/.libs:/home/ed/gnu/gcc-build/./gcc:/home/ed/gnu/gcc-build/./prev-gcc:/home/ed/gnu/gcc-build/./gmp/.libs:/home/ed/gnu/gcc-build/./prev-gmp/.libs:/home/ed/gnu/gcc-build/./mpfr/src/.libs:/home/ed/gnu/gcc-build/./prev-mpfr/src/.libs:/home/ed/gnu/gcc-build/./mpc/src/.libs:/home/ed/gnu/gcc-build/./prev-mpc/src/.libs:/home/ed/gnu/gcc-build/./isl/.libs:/home/ed/gnu/gcc-build/./prev-isl/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/src/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libsanitizer/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libmpx/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libvtv/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libcilkrts/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libssp/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libgo
 mp/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libitm/.libs:/home/ed/gnu/gcc-build/x86_64-pc-linux-gnu/libatomic/.libs:/home/ed/gnu/gcc-build/./gcc:/home/ed/gnu/gcc-build/./prev-gcc
> spawn [open ...]^M
> ./pr81162.exe: error while loading shared libraries: libubsan.so.0: cannot open
> shared object file: No such file or directory
> FAIL: gcc.dg/pr81162.c execution test

You need to move it to ubsan/
Comment 12 Bill Schmidt 2017-07-17 12:37:49 UTC
Right, sorry about the ubsan dependency screwup.  I'll move the test case later today.
Comment 13 Bill Schmidt 2017-07-17 19:12:43 UTC
Author: wschmidt
Date: Mon Jul 17 19:12:11 2017
New Revision: 250284

URL: https://gcc.gnu.org/viewcvs?rev=250284&root=gcc&view=rev
Log:
2017-07-17  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

	PR tree-optimization/81162
	* gcc.dg/pr81162.c: Move this to...
	* gcc.dg/ubsan/pr81162.c: ...here.


Added:
    trunk/gcc/testsuite/gcc.dg/ubsan/pr81162.c
Removed:
    trunk/gcc/testsuite/gcc.dg/pr81162.c
Modified:
    trunk/gcc/testsuite/ChangeLog
Comment 14 Bill Schmidt 2017-07-25 19:41:25 UTC
Author: wschmidt
Date: Tue Jul 25 19:40:50 2017
New Revision: 250542

URL: https://gcc.gnu.org/viewcvs?rev=250542&root=gcc&view=rev
Log:
[gcc]

2016-07-25  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

	Backport from mainline
	2016-07-14  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

	PR tree-optimization/81162
	* gimple-ssa-strength-reduction.c (replace_mult_candidate): Don't
	replace a negate with an add.

[gcc/testsuite]

2016-07-25  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

	Backport from mainline
	2016-07-14  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

	PR tree-optimization/81162
	* gcc.dg/ubsan/pr81162.c: New file.


Added:
    branches/gcc-7-branch/gcc/testsuite/gcc.dg/ubsan/pr81162.c
Modified:
    branches/gcc-7-branch/gcc/ChangeLog
    branches/gcc-7-branch/gcc/gimple-ssa-strength-reduction.c
    branches/gcc-7-branch/gcc/testsuite/ChangeLog
Comment 15 Bill Schmidt 2017-07-25 19:43:08 UTC
Author: wschmidt
Date: Tue Jul 25 19:42:36 2017
New Revision: 250543

URL: https://gcc.gnu.org/viewcvs?rev=250543&root=gcc&view=rev
Log:
[gcc]

2016-07-25  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

	Backport from mainline
	2016-07-14  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

	PR tree-optimization/81162
	* gimple-ssa-strength-reduction.c (replace_mult_candidate): Don't
	replace a negate with an add.

[gcc/testsuite]

2016-07-25  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

	Backport from mainline
	2016-07-14  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

	PR tree-optimization/81162
	* gcc.dg/ubsan/pr81162.c: New file.


Added:
    branches/gcc-6-branch/gcc/testsuite/gcc.dg/ubsan/pr81162.c
Modified:
    branches/gcc-6-branch/gcc/ChangeLog
    branches/gcc-6-branch/gcc/gimple-ssa-strength-reduction.c
    branches/gcc-6-branch/gcc/testsuite/ChangeLog
Comment 16 Bill Schmidt 2017-07-25 19:44:42 UTC
Author: wschmidt
Date: Tue Jul 25 19:44:10 2017
New Revision: 250544

URL: https://gcc.gnu.org/viewcvs?rev=250544&root=gcc&view=rev
Log:
[gcc]

2016-07-25  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

	Backport from mainline
	2016-07-14  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

	PR tree-optimization/81162
	* gimple-ssa-strength-reduction.c (replace_mult_candidate): Don't
	replace a negate with an add.

[gcc/testsuite]

2016-07-25  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

	Backport from mainline
	2016-07-14  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

	PR tree-optimization/81162
	* gcc.dg/ubsan/pr81162.c: New file.


Added:
    branches/gcc-5-branch/gcc/testsuite/gcc.dg/ubsan/pr81162.c
Modified:
    branches/gcc-5-branch/gcc/ChangeLog
    branches/gcc-5-branch/gcc/gimple-ssa-strength-reduction.c
    branches/gcc-5-branch/gcc/testsuite/ChangeLog
Comment 17 Bill Schmidt 2017-07-25 19:45:02 UTC
Now fixed everywhere.