Bug 89679 - [7 Regression] wrong code with -Og -frerun-cse-after-loop -fno-tree-fre
Summary: [7 Regression] wrong code with -Og -frerun-cse-after-loop -fno-tree-fre
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: rtl-optimization (show other bugs)
Version: 9.0
: P2 normal
Target Milestone: 7.5
Assignee: Jakub Jelinek
URL:
Keywords: wrong-code
Depends on:
Blocks: ohgee
  Show dependency treegraph
 
Reported: 2019-03-12 12:21 UTC by Zdenek Sojka
Modified: 2019-08-30 13:35 UTC (History)
3 users (show)

See Also:
Host: x86_64-pc-linux-gnu
Target: powerpc64le-unknown-linux-gnu
Build:
Known to work: 8.3.1, 9.0
Known to fail: 6.5.0, 7.4.1, 8.3.0
Last reconfirmed: 2019-03-12 00:00:00


Attachments
reduced testcase (180 bytes, text/plain)
2019-03-12 12:21 UTC, Zdenek Sojka
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Zdenek Sojka 2019-03-12 12:21:40 UTC
Created attachment 45948 [details]
reduced testcase

Output:
$ powerpc64le-unknown-linux-gnu-gcc -Og -frerun-cse-after-loop -fno-tree-fre testcase.c -static -Wall -W
$ ./a.out 
qemu: uncaught target signal 6 (Aborted) - core dumped
Aborted

$ powerpc64le-unknown-linux-gnu-gcc -v
Using built-in specs.
COLLECT_GCC=/repo/gcc-trunk/binary-latest-powerpc64le/bin/powerpc64le-unknown-linux-gnu-gcc
COLLECT_LTO_WRAPPER=/repo/gcc-trunk/binary-trunk-269602-checking-yes-rtl-df-extra-powerpc64le/bin/../libexec/gcc/powerpc64le-unknown-linux-gnu/9.0.1/lto-wrapper
Target: powerpc64le-unknown-linux-gnu
Configured with: /repo/gcc-trunk//configure --enable-languages=c,c++ --enable-valgrind-annotations --disable-nls --enable-checking=yes,rtl,df,extra --with-cloog --with-ppl --with-isl --with-sysroot=/usr/powerpc64le-unknown-linux-gnu --build=x86_64-pc-linux-gnu --host=x86_64-pc-linux-gnu --target=powerpc64le-unknown-linux-gnu --with-ld=/usr/bin/powerpc64le-unknown-linux-gnu-ld --with-as=/usr/bin/powerpc64le-unknown-linux-gnu-as --disable-libstdcxx-pch --prefix=/repo/gcc-trunk//binary-trunk-269602-checking-yes-rtl-df-extra-powerpc64le
Thread model: posix
gcc version 9.0.1 20190312 (experimental) (GCC)
Comment 1 Jakub Jelinek 2019-03-12 15:10:24 UTC
Started with r205060.
Comment 2 Jakub Jelinek 2019-03-12 15:24:35 UTC
The first thing which looks invalid to me is when fwprop1 adds a bogus REG_EQUAL note:
(insn 17 6 19 2 (set (reg:SI 140)
        (const_int -1 [0xffffffffffffffff])) "pr89679.c":16:3 494 {*movsi_internal1}
     (expr_list:REG_DEAD (reg:HI 138 [ c ])
        (expr_list:REG_DEAD (reg:HI 136 [ c ])
            (expr_list:REG_EQUAL (const_int 65535 [0xffff])
                (nil)))))

SImode -1 is certainly not equal to 0xffff.
Comment 3 Jakub Jelinek 2019-03-12 16:32:00 UTC
If I change inside of the debugger that 0xffff in REG_EQUAL note to constm1_rtx, then the testcase is not miscompiled.
The REG_EQUAL note is initially added by expand_mult_const:
      if (SCALAR_INT_MODE_P (mode))
        {
          /* Write a REG_EQUAL note on the last insn so that we can cse
             multiplication sequences.  Note that if ACCUM is a SUBREG,
             we've set the inner register and must properly indicate that.  */
          tem = op0, nmode = mode;
          accum_inner = accum;
          if (GET_CODE (accum) == SUBREG)
            {
              accum_inner = SUBREG_REG (accum);
              nmode = GET_MODE (accum_inner);
              tem = gen_lowpart (nmode, op0);
            }

          insn = get_last_insn ();
          wide_int wval_so_far
            = wi::uhwi (val_so_far,
                        GET_MODE_PRECISION (as_a <scalar_mode> (nmode)));
          rtx c = immed_wide_int_const (wval_so_far, nmode);
          set_dst_reg_note (insn, REG_EQUAL, gen_rtx_MULT (nmode, tem, c),
                            accum_inner);
        }
As accum is a (subreg:HI (reg:SI 140) 0), it emits a REG_EQUAL note with paradoxical subreg in it:
(insn 17 16 18 2 (set (reg:SI 140)
        (plus:SI (subreg:SI (reg:HI 138) 0)
            (subreg:SI (reg:HI 136) 0))) "pr89679.c":16:3 -1
     (expr_list:REG_EQUAL (mult:SI (subreg:SI (reg:HI 136) 0)
            (const_int 257 [0x101]))
        (nil)))
the note is already quite questionable, because the SImode register is not necessarily equal to that, the high 16 bits can be anything.
If it is not valid, then we either must make sure not to emit a REG_EQUAL note in that case or emit it on some insn that sets a HImode reg rather than SImode.
Note we emit that that way for quite a long time, I think since https://gcc.gnu.org/ml/gcc-patches/2000-12/msg00837.html

This is related to the https://gcc.gnu.org/ml/gcc-patches/2016-04/msg00418.html change, though in this case we already have such a REG_EQUAL note that before the change can be at least determined to have a paradoxical subreg in it and so REG_EQUAL note consumers could take that into account.
But if we simplify it into a CONST_INT or whatever else that doesn't have the paradoxical subreg in there anymore, this info is lost.

We could e.g. do:
--- gcc/fwprop.c.jj	2019-01-01 12:37:21.190908780 +0100
+++ gcc/fwprop.c	2019-03-12 17:24:14.623863160 +0100
@@ -1327,7 +1327,12 @@ forward_propagate_and_simplify (df_ref u
     {
       rtx note = find_reg_note (use_insn, REG_EQUAL, NULL_RTX);
       if (DF_REF_FLAGS (use) & DF_REF_IN_NOTE)
-	loc = &XEXP (note, 0);
+	{
+	  loc = &XEXP (note, 0);
+	  /* Don't simplify a paradoxical SUBREG in a REG_EQUAL note.  */
+	  if (paradoxical_subreg_p (DF_REF_REG (use)))
+	    return false;
+	}
       else
 	loc = &SET_SRC (use_set);
 
and thus not really simplify paradoxical subregs in REG_EQUAL notes, the notes will be then removed if we DCE the setters of corresponding pseudos.

Thoughts on this?
Comment 4 Jakub Jelinek 2019-03-12 16:41:15 UTC
The other variant would be perhaps more in line with the PR70574 change where we don't add REG_EQUAL notes with paradoxical subregs in it, by:
--- gcc/expmed.c.jj	2019-01-10 11:43:14.387377695 +0100
+++ gcc/expmed.c	2019-03-12 17:38:55.286511666 +0100
@@ -3356,13 +3356,20 @@ expand_mult_const (machine_mode mode, rt
 	      tem = gen_lowpart (nmode, op0);
 	    }
 
-	  insn = get_last_insn ();
-	  wide_int wval_so_far
-	    = wi::uhwi (val_so_far,
-			GET_MODE_PRECISION (as_a <scalar_mode> (nmode)));
-	  rtx c = immed_wide_int_const (wval_so_far, nmode);
-	  set_dst_reg_note (insn, REG_EQUAL, gen_rtx_MULT (nmode, tem, c),
-			    accum_inner);
+	  /* Don't add a REG_EQUAL note if tem is a paradoxical SUBREG.
+	     In that case, only the low bits of accum would be guaranteed to
+	     be equal to the content of the REG_EQUAL note, the upper bits
+	     can be anything.  */
+	  if (!paradoxical_subreg_p (tem))
+	    {
+	      insn = get_last_insn ();
+	      wide_int wval_so_far
+		= wi::uhwi (val_so_far,
+			    GET_MODE_PRECISION (as_a <scalar_mode> (nmode)));
+	      rtx c = immed_wide_int_const (wval_so_far, nmode);
+	      set_dst_reg_note (insn, REG_EQUAL, gen_rtx_MULT (nmode, tem, c),
+				accum_inner);
+	    }
 	}
     }
Comment 5 Eric Botcazou 2019-03-12 17:43:26 UTC
> The other variant would be perhaps more in line with the PR70574 change
> where we don't add REG_EQUAL notes with paradoxical subregs in it.

Yes, that looks like the right thing to do to me.
Comment 6 Jakub Jelinek 2019-03-14 12:22:07 UTC
Author: jakub
Date: Thu Mar 14 12:21:36 2019
New Revision: 269680

URL: https://gcc.gnu.org/viewcvs?rev=269680&root=gcc&view=rev
Log:
	PR rtl-optimization/89679
	* expmed.c (expand_mult_const): Don't add a REG_EQUAL note if it
	would contain a paradoxical SUBREG.

	* gcc.dg/pr89679.c: New test.

Added:
    trunk/gcc/testsuite/gcc.dg/pr89679.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/expmed.c
    trunk/gcc/testsuite/ChangeLog
Comment 7 Jakub Jelinek 2019-03-14 12:23:35 UTC
Fixed on the trunk so far.
Comment 8 Jakub Jelinek 2019-04-30 20:45:20 UTC
Author: jakub
Date: Tue Apr 30 20:44:48 2019
New Revision: 270730

URL: https://gcc.gnu.org/viewcvs?rev=270730&root=gcc&view=rev
Log:
	Backported from mainline
	2019-03-14  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/89679
	* expmed.c (expand_mult_const): Don't add a REG_EQUAL note if it
	would contain a paradoxical SUBREG.

	* gcc.dg/pr89679.c: New test.

Added:
    branches/gcc-8-branch/gcc/testsuite/gcc.dg/pr89679.c
Modified:
    branches/gcc-8-branch/gcc/ChangeLog
    branches/gcc-8-branch/gcc/expmed.c
    branches/gcc-8-branch/gcc/testsuite/ChangeLog
Comment 9 Jakub Jelinek 2019-05-01 07:12:46 UTC
Fixed for 8.4+ too.
Comment 10 Jakub Jelinek 2019-08-30 12:20:04 UTC
Author: jakub
Date: Fri Aug 30 12:19:33 2019
New Revision: 275132

URL: https://gcc.gnu.org/viewcvs?rev=275132&root=gcc&view=rev
Log:
	Backported from mainline
	2019-03-14  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/89679
	* expmed.c (expand_mult_const): Don't add a REG_EQUAL note if it
	would contain a paradoxical SUBREG.

	* gcc.dg/pr89679.c: New test.

Added:
    branches/gcc-7-branch/gcc/testsuite/gcc.dg/pr89679.c
Modified:
    branches/gcc-7-branch/gcc/ChangeLog
    branches/gcc-7-branch/gcc/expmed.c
    branches/gcc-7-branch/gcc/testsuite/ChangeLog
Comment 11 Jakub Jelinek 2019-08-30 13:35:38 UTC
Fixed.