User account creation filtered due to spam.

Bug 48156 - [4.6 Regression] wrong code with -fcrossjumping
Summary: [4.6 Regression] wrong code with -fcrossjumping
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: rtl-optimization (show other bugs)
Version: 4.7.0
: P3 normal
Target Milestone: 4.6.1
Assignee: Jakub Jelinek
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2011-03-16 20:34 UTC by Zdenek Sojka
Modified: 2011-03-26 09:28 UTC (History)
4 users (show)

See Also:
Host: x86_64-pc-linux-gnu
Target: x86_64-pc-linux-gnu
Build:
Known to work: 4.5.3, 4.6.1, 4.7.0
Known to fail: 4.6.0
Last reconfirmed: 2011-03-16 23:59:44


Attachments
reduced testcase (235 bytes, text/plain)
2011-03-16 20:34 UTC, Zdenek Sojka
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Zdenek Sojka 2011-03-16 20:34:58 UTC
Created attachment 23684 [details]
reduced testcase

Output:
$ gcc -O -fcrossjumping --param min-crossjump-insns=1 testcase.c
$ ./a.out 
Aborted

Tested revisions:
r171047 - fail
4.6 r170955 - fail
4.5 r170955 - OK
Comment 1 Steven Bosscher 2011-03-17 00:24:52 UTC
Cross-jumping changes this:

   24 NOTE_INSN_BASIC_BLOCK
   25 si:SI=bx:SI
      REG_DEAD: bx:SI
   26 di:SI=0x8
   27 call <...>
      REG_DEAD: di:SI
      REG_DEAD: si:SI
      REG_EH_REGION: 0
   48 pc=L37
i  49: barrier
L30:  
   31 NOTE_INSN_BASIC_BLOCK
   32 si:SI=bx:SI
      REG_DEAD: bx:SI
   33 di:SI=0x4 
   34 call <...>
      REG_DEAD: di:SI
      REG_DEAD: si:SI
      REG_EH_REGION: 0
L37:
   38 NOTE_INSN_BASIC_BLOCK
   55 NOTE_INSN_EPILOGUE_BEG


to this:

   24 NOTE_INSN_BASIC_BLOCK
   26 di:SI=0x8
   62 pc=L61
i  63: barrier
L30:
   31 NOTE_INSN_BASIC_BLOCK
   33 di:SI=0x4
L61:
   59 NOTE_INSN_BASIC_BLOCK
   34 call <...>
      REG_DEAD: di:SI
      REG_DEAD: si:SI
      REG_EH_REGION: 0
   55 NOTE_INSN_EPILOGUE_BEG

So the call is unified, and that looks OK to me. Investigating...
Comment 2 Jakub Jelinek 2011-03-18 10:34:16 UTC
Started with
http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=164552
Comment 3 Jakub Jelinek 2011-03-18 10:50:29 UTC
The problem is not in %rdx, but %rsi.  It was originally initialized from %rbx right before the call, but now is the %rsi assignment moved before the testing of the j && equals (i, j) condition, which is wrong, because %rsi is call clobbered and thus potentionally clobbered by the call, and more importantly even set right before the call to equals function.
Comment 4 Jakub Jelinek 2011-03-18 12:42:33 UTC
The problem is that try_head_merge_bb uses DF info that isn't up to date after previous crossjumping and hasn't been updated yet.

In particular when we compute live_union, one merge_bb is:
;; basic block 4, loop depth 0, count 0
;; prev block 3, next block 5
;; pred:       3 [20.7%]  (fallthru)
;; succ:       7 [100.0%] 
;; bb 4 artificial_defs: { }
;; bb 4 artificial_uses: { u-1(7){ }}
;; lr  in  	 3 [bx] 7 [sp]
;; lr  use 	 3 [bx] 7 [sp]
;; lr  def 	 0 [ax] 1 [dx] 2 [cx] 4 [si] 5 [di] 8 [st] 9 [st(1)] 10 [st(2)] 11 [st(3)] 12 [st(4)] 13 [st(5)] 14 [st(6)] 15 [st(7)] 17 [flags] 18 [fpsr] 19 [fpcr] 21 [xmm0] 22 [xmm1] 23 [xmm2] 24 [xmm3] 25 [xmm4] 26 [xmm5] 27 [xmm6] 28 [xmm7] 29 [mm0] 30 [mm1] 31 [mm2] 32 [mm3] 33 [mm4] 34 [mm5] 35 [mm6] 36 [mm7] 37 [r8] 38 [r9] 39 [r10] 40 [r11] 45 [xmm8] 46 [xmm9] 47 [xmm10] 48 [xmm11] 49 [xmm12] 50 [xmm13] 51 [xmm14] 52 [xmm15]

(note 24 23 25 4 [bb 4] NOTE_INSN_BASIC_BLOCK)
(insn 25 24 26 4 (set (reg:SI 4 si)
        (reg/v:SI 3 bx [orig:59 i ] [59])) pr48156.c:31 64 {*movsi_internal}
     (expr_list:REG_DEAD (reg/v:SI 3 bx [orig:59 i ] [59])
        (nil)))
(insn 26 25 62 4 (set (reg:SI 5 di)
        (const_int 8 [0x8])) pr48156.c:31 64 {*movsi_internal}
     (nil))
(jump_insn 62 26 63 4 (set (pc)
        (label_ref 61)) -1
     (nil)
 -> 61)
;; lr  out 	 7 [sp]

where lr_out is obviously wrong, lr out should also include 4 [si] 5 [di].
That lr_out comes from the bb that originally also included the call to baz which consumed and made dead those two registers.  The other merge_bb has exactly the same problem.  bb 7 (the new bb which contains just the call to baz)
has lr in/use/def/out all empty.

So I guess we need to tell DF to recompute it after each successful crossjumping or recompute it somehow manually.
Comment 5 Jakub Jelinek 2011-03-18 12:45:35 UTC
Steven, sorry for looking at a bug you have assigned, I've been just curious (especially in the light if it should be a 4.6 blocker or not).
I'll leave you DF folks now to sort it out.
Comment 6 Jakub Jelinek 2011-03-18 13:32:13 UTC
Actually, I think the bug is in df_get_bb_dirty:

--- gcc/df-core.c.jj	2010-12-14 08:11:39.000000000 +0100
+++ gcc/df-core.c	2011-03-18 14:22:43.000000000 +0100
@@ -1400,10 +1400,16 @@ df_mark_solutions_dirty (void)
 bool
 df_get_bb_dirty (basic_block bb)
 {
-  if (df && df_live)
-    return bitmap_bit_p (df_live->out_of_date_transfer_functions, bb->index);
-  else
-    return false;
+  if (df)
+    {
+      if (df_live)
+	return bitmap_bit_p (df_live->out_of_date_transfer_functions,
+			     bb->index);
+      else if (df_lr)
+	return bitmap_bit_p (df_lr->out_of_date_transfer_functions,
+			     bb->index);
+    }
+  return false;
 }
 
Both ifcvt.c and cfgcleanup.c via simulate_backwards_to_point call and use df_get_live_{in,out} (), which is:
  if (df_live)
    return DF_LIVE_OUT (bb);
  else
    return DF_LR_OUT (bb);
On this testcase, df_live is NULL during crossjumping, so df_get_bb_dirty always returns false, even when the solution it is using is dirty.  With the patch above cfgcleanup.c does the right thing, bails out from the merging when it is dirty and will df_analyze () afterwards and loop (as the dirtyness was caused by changes).
Comment 7 Jakub Jelinek 2011-03-18 14:14:15 UTC
Another option is:

--- gcc/combine-stack-adj.c	2010-12-02 11:51:32.000000000 +0100
+++ gcc/combine-stack-adj.c	2011-03-18 15:12:09.497674812 +0100
@@ -551,7 +551,16 @@ gate_handle_stack_adjustments (void)
 static unsigned int
 rest_of_handle_stack_adjustments (void)
 {
-  cleanup_cfg (flag_crossjumping ? CLEANUP_CROSSJUMP : 0);
+  if (flag_crossjumping && optimize <= 1)
+    {
+      df_live_add_problem ();
+      df_live_set_all_dirty ();
+      df_analyze ();
+      cleanup_cfg (CLEANUP_CROSSJUMP);
+      df_remove_problem (df_live);
+    }
+  else
+    cleanup_cfg (flag_crossjumping ? CLEANUP_CROSSJUMP : 0);
 
   /* This is kind of a heuristic.  We need to run combine_stack_adjustments
      even for machines with possibly nonzero TARGET_RETURN_POPS_ARGS

ifcvt.c already does something similar, so the df_get_bb_dirty calls it does always return whether the bb is dirty in df_live.
Comment 8 Paolo Bonzini 2011-03-18 16:24:25 UTC
I like the patch from comment 6.
Comment 9 Jakub Jelinek 2011-03-20 15:25:59 UTC
Author: jakub
Date: Sun Mar 20 15:25:55 2011
New Revision: 171195

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=171195
Log:
	PR rtl-optimization/48156
	* df-core.c (df_get_bb_dirty): Use df_lr if df_live is NULL,
	assume df and df_lr are not NULL.

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

Added:
    trunk/gcc/testsuite/gcc.dg/pr48156.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/df-core.c
    trunk/gcc/testsuite/ChangeLog
Comment 10 Jakub Jelinek 2011-03-20 15:34:46 UTC
Fixed on the trunk so far.
Comment 11 Jakub Jelinek 2011-03-26 09:21:36 UTC
Author: jakub
Date: Sat Mar 26 09:21:34 2011
New Revision: 171547

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=171547
Log:
	Backport from mainline
	2011-03-20  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/48156
	* df-core.c (df_get_bb_dirty): Use df_lr if df_live is NULL,
	assume df and df_lr are not NULL.

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

Added:
    branches/gcc-4_6-branch/gcc/testsuite/gcc.dg/pr48156.c
Modified:
    branches/gcc-4_6-branch/gcc/ChangeLog
    branches/gcc-4_6-branch/gcc/df-core.c
    branches/gcc-4_6-branch/gcc/testsuite/ChangeLog
Comment 12 Jakub Jelinek 2011-03-26 09:28:22 UTC
Fixed.