This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.

Index Nav:	[Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav:	[Date Prev] [Date Next]	[Thread Prev] [Thread Next]
Other format:	[Raw text]

Re: [SMS] Schedule normalization after scheduling branch

From: Martin Sebor <msebor at gmail dot com>
To: Roman Zhuykov <zhroma at ispras dot ru>, gcc-patches <gcc-patches at gcc dot gnu dot org>
Cc: Dmitry Melnik <dm at ispras dot ru>, msebor at gcc dot gnu dot org, abel at gcc dot gnu dot org, amonakov at gcc dot gnu dot org, jakub at gcc dot gnu dot org
Date: Wed, 20 Jan 2016 14:30:23 -0700
Subject: Re: [SMS] Schedule normalization after scheduling branch
Authentication-results: sourceware.org; auth=none
References: <CAJnFk2eiT9SOPQwaAe8KqkMvx-fD_0YrJ1YqvF9NC5ny2phE1g at mail dot gmail dot com> <CAJnFk2dZcbwyHsqa3Nboxznm=EBmxZWBssnW0T5ng7P7hPSJMA at mail dot gmail dot com>

On 01/18/2016 02:38 PM, Roman Zhuykov wrote:

Hello,

4 years ago when I create some SMS patches nobody cares about SMS
failures on ia64.  Now ia64 is even more dead but at least one of bugs
appears on powerpc - PR69252.
Proposed patch is here:
https://gcc.gnu.org/ml/gcc-patches/2011-12/txt00266.txt and it even
suits current trunk without modification.

Powerpc PR69252 situation seems to be the same as I described earlier
on ia64, I can discuss it once again.

There were the following doloop:

   set_before reg
Loop:
   use1 reg
   set reg
   use2 reg
   insn
   cloop

After SMS it looked like this (I add scheduling stage and cycle before
each loop instruction):

   set_before reg
SMS-prologue:
   set reg
   use1 reg_copy #uninitialized
   use2 reg
   reg_copy <- reg
Loop:
0  0 set reg
0  0 use1 reg_copy
0  4 use2 reg
0 -4 reg_copy <- reg
0  8 insn
0 -1 cloop

All instructions were wrongly classified to stage zero.  While copying
them to prologue the regmove remains to be placed after use1, and as a
result, the register reg_copy is used uninititalized in prologue.
This leads to miscompilation.

I have found that the issue can be fixed by additional schedule
normalizarion after scheduling branch instruction in optimize_sc
function.  The situation here is the same as in patch by Richard
Sandiford
http://gcc.gnu.org/ml/gcc-patches/2011-10/msg00748.html which enables
scheduling regmoves.  "Moves that handle incoming values might have
been added to a new first stage.  Bump the stage count if so."  The
same bumping should be done after scheduling branch, and my patch
simply implements this.

As Martin Sebor have already done bootstrap and regtest in PR69252, I
want to ask him to attach PR69252 example as a new regression test to
this patch.  I don't know if it should be execution test or maybe
there are some special scan-assembler techniques.

Ok for trunk with regtest?


A patch with the regression test is attached.  I verified that
the test fails without your patch and passes with it applied.

Martin


--
Roman

Here is the whole old letter about the patch:
2011-12-29 17:04 GMT+03:00 Roman Zhuykov <zhroma@ispras.ru>:

This week I investigated modulo scheduler on IA64.  Enabling SMS by default
(-fmodulo-sched -fmodulo-sched-allow-regmoves) leads to bootstrap failure
on IA64: gcc/build/genautomata.o differs while comparing stages 2 and 3.

I haven't studied this issue in detail, because the combination of these
my patches fixes this problem:

[Additional edges to instructions with clobber]
http://gcc.gnu.org/ml/gcc-patches/2011-12/msg00505.html
[Correctly delete anti-dep edges for renaming]
http://gcc.gnu.org/ml/gcc-patches/2011-12/msg00506.html

Then I have regtested two compilers - first is clean trunk, and the second
is trunk with SMS enabled by default and two patches mentioned above.
Comparing the results shows several new failures.

FAIL: gcc.dg/pr45259.c (internal compiler error)
FAIL: gcc.dg/pr45259.c (test for excess errors)
FAIL: gcc.dg/pr47881.c (test for excess errors)
FAIL: tr1/5_numerical_facilities/special_functions/08_cyl_bessel_i/check_value.cc
execution test
FAIL: tr1/5_numerical_facilities/special_functions/09_cyl_bessel_j/check_value.cc
execution test
FAIL: tr1/5_numerical_facilities/special_functions/11_cyl_neumann/check_value.cc
execution test
FAIL: tr1/5_numerical_facilities/special_functions/21_sph_bessel/check_value.cc
execution test
FAIL: tr1/5_numerical_facilities/special_functions/23_sph_neumann/check_value.cc
execution test

Problem with gcc.dg/pr45259.c is an ICE, which I earlier fixed by this patch:
[Correct extracting loop exit condition]
http://gcc.gnu.org/ml/gcc-patches/2011-09/msg02049.html

In gcc.dg/pr47881.c -fcompare-debug failure happens. The difference between
-fcompare-debug dumps is only some NOTE_INSN_DELETED entries are placed
differently.  I haven't studied this problem.

And the last 5 new failures have dissappered after fixing the following
described issue.

Imagine the following doloop (each use and set is a fmad in real example):

use1 reg
set reg
use2 reg
insn
cloop

After SMS it looks like this, I write a scheduling stage and cycle before each
instruction.

0  0 set reg
0  0 use1 reg_copy
0  4 use2 regR
0 -4 reg_copy = reg
0  8 insn
0 -1 cloop

So all instructions were wrongly classified to stage zero.  While copying them
to prologue the regmove remains to be placed after use1, and as a
result, the register
reg_copy is used uninititalized in prologue.  This leads to miscompilation.

I have found that the issue can be fixed by additional schedule normalizarion
after scheduling branch instruction in optimize_sc function.  The situation
here is the same as in patch by Richard Sandiford
http://gcc.gnu.org/ml/gcc-patches/2011-10/msg00748.html
which enables scheduling regmoves.
"Moves that handle incoming values might have been added
to a new first stage.  Bump the stage count if so."
The same bumping should be done after scheduling branch.

In my model example branch is scheduled on cycle -1 and remains in zero stage.
When regmove is later scheduled on cycle -4 the Richard's check doesn't cause
normalization, because new PS_MIN_CYCLE is -4, but min_cycle is -12:

   min_cycle = PS_MIN_CYCLE (ps) - SMODULO (PS_MIN_CYCLE (ps), ps->ii);
   ...
   call schedule_reg_moves (ps)
   ...
   if (PS_MIN_CYCLE (ps) < min_cycle)
     {
       reset_sched_times (ps, 0);
       stage_count++;
     }

I attach patch which adds the same check into optimize_sc function.
With -fmodulo-sched -fmodulo-sched-allow-regmoves enabled it passes
bootstrap and regtest on IA64.  It also passes bootstrap and regtest
on x86_64 with SMS patched to schedule non-doloop loops.
[Support new loop pattern]
http://gcc.gnu.org/ml/gcc-patches/2011-12/msg00495.html

OK for trunk or maybe 4.8?

Happy holidays!

--
Roman Zhuykov

2016-01-20  Martin Sebor  <msebor@redhat.com>

	PR target/69252
	* gcc.target/powerpc/pr69252.c: New test.

diff --git a/gcc/testsuite/gcc.target/powerpc/pr69252.c b/gcc/testsuite/gcc.target/powerpc/pr69252.c
new file mode 100644
index 0000000..23334a9
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr69252.c
@@ -0,0 +1,28 @@
+/* PR target/69252 - [4.9/5/6 Regression] gcc.dg/vect/vect-iv-9.c FAILs
+   with -Os -fmodulo-sched -fmodulo-sched-allow-regmoves -fsched-pressure  */
+/* { dg-do run } */
+/* { dg-options "-Os -fmodulo-sched -fmodulo-sched-allow-regmoves -fsched-pressure " } */
+#define N 26
+int a[N];
+__attribute__ ((noinline, noclone))
+     int main1 (int X)
+{
+  int s = X;
+  int i;
+  for (i = 0; i < N; i++)
+    s += (i + a[i]);
+  return s;
+}
+
+int
+main (void)
+{
+  int s, i;
+  for (i = 0; i < N; i++)
+    a[i] = 2 * i;
+  s = main1 (3);
+  if (s != 978)
+    __builtin_abort ();
+  return 0;
+}
+

Follow-Ups:
- Re: [SMS] Schedule normalization after scheduling branch
  - From: Jeff Law

References:
- Re: [SMS] Schedule normalization after scheduling branch
  - From: Roman Zhuykov

Index Nav:	[Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav:	[Date Prev] [Date Next]	[Thread Prev] [Thread Next]