Bug 46614

Created attachment 22489 [details]
auto-reduced testcase

$ gcc -O -fgcse -fno-rename-registers -fsched2-use-superblocks -fschedule-insns2 -ftree-vectorize -funroll-loops testcase.c
$ ./a.out
Aborted

Created attachment 22490 [details]
more reduced testcase

This one needs less flags, but it gives warnings with -Wall -W - though they should be harmless.

$ gcc -O -fno-rename-registers -fsched2-use-superblocks -fschedule-insns2 -funroll-loops testcase2.c -Wall -W
testcase2.c: In function 'main1':
testcase2.c:30:13: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
testcase2.c:31:13: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
testcase2.c:38:20: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
$ ./a.out 
Aborted

It is caused by revision 154667:

http://gcc.gnu.org/ml/gcc-cvs/2009-11/msg00890.html

Created attachment 22492 [details]
pr46614.c

Adjusted testcase which reports no warnings.  Looking into it...

It looks that sched2 pass is to blame. For the original case, we have:

_.split4:

;; Function main1 (main1)
...
  207 [sp:DI+0x30]=xmm8:V16QI
      REG_DEAD: xmm8:V16QI
 1158 xmm1:V16QI=xmm6:V16QI
  208 xmm1:V16QI=vec_select
      REG_EQUIV: [frame:DI-0x60]
  209 [sp:DI+0x40]=xmm1:V16QI
      REG_DEAD: xmm1:V16QI
  210 xmm6:V16QI=vec_select
      REG_DEAD: xmm5:V16QI
      REG_EQUIV: [frame:DI-0x50]
  211 [sp:DI+0x50]=xmm6:V16QI
      REG_DEAD: xmm6:V16QI
...
  468 NOTE_INSN_BASIC_BLOCK
 1327 r12:SI=zero_extend([di:DI+0x35])
 1328 r13:SI=zero_extend([sp:DI+0x50])
  471 {bp:SI=bp:SI+r12:SI;clobber flags:CC;}
...

_.sched2:

;; Function main1 (main1)
...
 1406 NOTE_INSN_PROLOGUE_END
  223 dx:QI=[di:DI+0x2]
  221 ax:QI=[di:DI+0x1]
    8 xmm2:V16QI=unspec[[di:DI+0x11]] 40
 1339 r12:SI=zero_extend([sp:DI+0x48])
 1328 r13:SI=zero_extend([sp:DI+0x50])
    6 xmm1:V16QI=unspec[[di:DI+0x1]] 40
...
  211 [sp:DI+0x50]=xmm6:V16QI
      REG_DEAD: xmm6:V16QI
 1383 r10:SI=zero_extend([sp:DI+0x28])
  209 [sp:DI+0x40]=xmm1:V16QI
      REG_DEAD: xmm1:V16QI
...

See how (insn 1339) and (insn 1328) passed (insn 209) and (insn 211).

Yeah, on the #c4 testcase it works with additional
--param max-pending-list-length=33
(compared to the default 32), so it is related to the flush of pending list.

So, the bug is that we have two kinds of INSNs in deps->last_pending_memory_flush
1) jumps added there via:
            deps->last_pending_memory_flush
              = alloc_INSN_LIST (insn, deps->last_pending_memory_flush);
   in deps_analyze_insn
2) any instructions added there via:
      deps->last_pending_memory_flush = alloc_INSN_LIST (insn, NULL_RTX);
   in flush_pending_lists

This list is used to add dependencies in flush_pending_lists (correct), sched_analyze_1 (for writes, also correct, and one correct for write spot in sched_analyze_insn) and then in sched_analyze_2 and sched_analyze_insn for reads/DEBUG_INSNs, where it differentiates between 1) and 2) above using JUMP_P test on the insn in the last_pending_memory_flush list.  E.g. in sched_analyze_2
it adds deps always for the (assumed) category 2) and only adds something for category 1) if it the memory can trap.  The problem is that JUMP_P is not a good
test between 1) and 2), as flush_pending_lists can be also called on JUMP_Ps:
      /* Flush pending lists on jumps, but not on speculative checks.  */
      if (JUMP_P (insn) && !(sel_sched_p ()
                             && sel_insn_is_speculation_check (insn)))
        flush_pending_lists (deps, insn, true, true);
and (what happens here):
      if (!deps->readonly
          && JUMP_P (insn)
          && !(sel_sched_p ()
               && sel_insn_is_speculation_check (insn)))
        {
          /* Keep the list a reasonable size.  */
          if (deps->pending_flush_length++ > MAX_PENDING_LIST_LENGTH)
            flush_pending_lists (deps, insn, true, true);
          else
            deps->last_pending_memory_flush
              = alloc_INSN_LIST (insn, deps->last_pending_memory_flush);
        }

What happens on the testcase is that pending_flush_length gets too large and
so we flush_pending_lists and flushes everything, adding needed dependencies
and sets deps->last_pending_memory_flush to contain just that jump.
A few insns later we process a read and go through last_pending_memory_flush,
see it only contains a JUMP_P and that the memory can't trap and don't add any dependency.  But in this case, because the jump was added as result of flush_pending_lists and thus represents all the memory writes before it as well,
it is wrong, we need to add a dependency.

I guess we need to keep the information whether a jump was added as part of flush_pending_lists or for 1) somewhere, wonder if mode of the INSN_LIST wouldn't be usable in this case for it.

Created attachment 22497 [details]
gcc46-pr46614.patch

Untested fix that cures this testcase.

Author: jakub
Date: Wed Nov 24 16:56:44 2010
New Revision: 167121

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=167121
Log:
	PR rtl-optimization/46614
	* sched-deps.c (NON_FLUSH_JUMP_KIND, NON_FLUSH_JUMP_P): Define.
	 (deps_analyze_insn): Mark JUMP_INSNs in
	last_pending_memory_flush that weren't added through
	flush_pending_lists with NON_FLUSH_JUMP_KIND.
	(sched_analyze_2, sched_analyze_insn): Check NON_FLUSH_JUMP_P
	on INSN_LIST instead of JUMP_P check on its operand.
	* sched-rgn.c (concat_INSN_LIST): Copy over REG_NOTE_KIND.

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

Added:
    trunk/gcc/testsuite/gcc.dg/pr46614.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/sched-deps.c
    trunk/gcc/sched-rgn.c
    trunk/gcc/testsuite/ChangeLog

Fixed on the trunk so far.

Author: jakub
Date: Tue Dec  7 15:25:48 2010
New Revision: 167546

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=167546
Log:
	Backport from mainline
	2010-11-24  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/46614
	* sched-deps.c (NON_FLUSH_JUMP_KIND, NON_FLUSH_JUMP_P): Define.
	 (deps_analyze_insn): Mark JUMP_INSNs in
	last_pending_memory_flush that weren't added through
	flush_pending_lists with NON_FLUSH_JUMP_KIND.
	(sched_analyze_2, sched_analyze_insn): Check NON_FLUSH_JUMP_P
	on INSN_LIST instead of JUMP_P check on its operand.
	* sched-rgn.c (concat_INSN_LIST): Copy over REG_NOTE_KIND.

	PR rtl-optimization/46614
	* gcc.dg/pr46614.c: New test.

Added:
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/pr46614.c
Modified:
    branches/gcc-4_5-branch/gcc/ChangeLog
    branches/gcc-4_5-branch/gcc/sched-deps.c
    branches/gcc-4_5-branch/gcc/sched-rgn.c
    branches/gcc-4_5-branch/gcc/testsuite/ChangeLog

Fixed.