Correct fix for scheduler bug PR11320

Correct fix for scheduler bug PR11320

[Bernd Schmidt]

[-- Attachment #1: Type: text/plain, Size: 2341 bytes --]

PR11320 was a scheduler problem where an instruction was moved backwards
across a branch, leading to
	addl r14 = @ltoffx(.LC2), r1
	;;
	(p7) addl r14 = 1, r0		<--- r14 clobbered
	;;
	(p7) st4 [r15] = r14
	ld8.mov r14 = [r14], .LC2	<--- crash
	(branch was here)

At the time, this was solved by a patch that recognizes when a register
is conditionally set in a basic block, and then treats the branch as
setting such a register:

http://gcc.gnu.org/ml/gcc-patches/2003-07/msg01044.html

"The proposed fix is to say that JUMP_INSNs set the registers that are
live on entry of the fallthrough block and are conditionally set before
the jump, because they can be considered as restoring the former value
of the conditionally set registers."

While it is true that a jump could be seen as setting the correct value
for a register, this isn't at all related to conditional sets. Consider
a trivial example:

a = b + 3;
if (b > 0) { use a } else { use a }

where the value of a is different in each branch of the else, with no
conditional execution in sight. From the point of view of the uses, it
can be argued that the jump sets the value, but this is irrelevant for
scheduling purposes.

There's nothing wrong with using a wrong value in an instruction hoisted
across a branch if the result of that instruction will (eventually) be
dead. We do this all the time in sched-ebb. The patch needlessly
restricts scheduling opportunities and should be reverted.

The real problem here is that the ia64 backend lies to the rest of the
compiler; it produces a load instruction without showing a MEM anywhere
in the instruction pattern. Hence, the following patch, which reverts
the bogus scheduler changes and adds a MEM to a pattern in ia64.md. The
represenation is somewhat weird, but no more so than before where the
load was represented as a plain LO_SUM. The documentation I have doesn't
mention ld8.mov, so I'll have to leave it to the ia64 maintainers to
choose between this change or implement something more involved.

Bootstrapped and tested without regressions on ia64-linux with
languages=c,c++,obj,fortran and --disable-shared; that was what I could
get to work at all at the moment. Even then, C++ seems pretty broken.
I've also built a 3.3 cross-cc1 to see if the original bug is still
fixed (it is).

Ok (scheduler & ia64 bits)?


Bernd

[-- Attachment #2: pr11320.diff --]
[-- Type: text/plain, Size: 9898 bytes --]

	Revert
	2003-07-10  Eric Botcazou  <ebotcazou@libertysurf.fr>

	PR optimization/11320
	* sched-int.h (struct deps) [reg_conditional_sets]: New field.
	(struct sched_info) [compute_jump_reg_dependencies]: New prototype.
	* sched-deps.c (sched_analyze_insn) [JUMP_INSN]: Update call to
	current_sched_info->compute_jump_reg_dependencies. Record which
	registers are used and which registers are set by the jump.
	Clear deps->reg_conditional_sets after a barrier.
	Set deps->reg_conditional_sets if the insn is a COND_EXEC.
	Clear deps->reg_conditional_sets if the insn is not a COND_EXEC.
	(init_deps): Initialize reg_conditional_sets.
	(free_deps): Clear reg_conditional_sets.
	* sched-ebb.c (compute_jump_reg_dependencies): New prototype.
	Mark registers live on entry of the fallthrough block and conditionally
	set as set by the jump. Mark registers live on entry of non-fallthrough
	blocks as used by the jump.
	* sched-rgn.c (compute_jump_reg_dependencies): New prototype.
	Mark new parameters as unused.

	* config/ia64/ia64.md (load_symptr_low): Show a MEM.
	* config/ia64/ia64.c (ia64_expand_load_address): Generate it.

Index: gcc/sched-ebb.c
===================================================================
--- gcc/sched-ebb.c	(revision 176195)
+++ gcc/sched-ebb.c	(working copy)
@@ -238,28 +238,18 @@
   return 1;
 }
 
- /* INSN is a JUMP_INSN, COND_SET is the set of registers that are
-    conditionally set before INSN.  Store the set of registers that
-    must be considered as used by this jump in USED and that of
-    registers that must be considered as set in SET.  */
+ /* INSN is a JUMP_INSN.  Store the set of registers that
+    must be considered as used by this jump in USED.  */
 
 void
-ebb_compute_jump_reg_dependencies (rtx insn, regset cond_set, regset used,
-				   regset set)
+ebb_compute_jump_reg_dependencies (rtx insn, regset used)
 {
   basic_block b = BLOCK_FOR_INSN (insn);
   edge e;
   edge_iterator ei;
 
   FOR_EACH_EDGE (e, ei, b->succs)
-    if (e->flags & EDGE_FALLTHRU)
-      /* The jump may be a by-product of a branch that has been merged
-	 in the main codepath after being conditionalized.  Therefore
-	 it may guard the fallthrough block from using a value that has
-	 conditionally overwritten that of the main codepath.  So we
-	 consider that it restores the value of the main codepath.  */
-      bitmap_and (set, df_get_live_in (e->dest), cond_set);
-    else
+    if ((e->flags & EDGE_FALLTHRU) == 0)
       bitmap_ior_into (used, df_get_live_in (e->dest));
 }
 
Index: gcc/modulo-sched.c
===================================================================
--- gcc/modulo-sched.c	(revision 176195)
+++ gcc/modulo-sched.c	(working copy)
@@ -252,9 +252,7 @@
 
 static void
 compute_jump_reg_dependencies (rtx insn ATTRIBUTE_UNUSED,
-			       regset cond_exec ATTRIBUTE_UNUSED,
-			       regset used ATTRIBUTE_UNUSED,
-			       regset set ATTRIBUTE_UNUSED)
+			       regset used ATTRIBUTE_UNUSED)
 {
 }
 
Index: gcc/sched-deps.c
===================================================================
--- gcc/sched-deps.c	(revision 176195)
+++ gcc/sched-deps.c	(working copy)
@@ -568,7 +568,7 @@
 	  (rev1==rev2
 	  ? reversed_comparison_code (cond2, NULL)
 	  : GET_CODE (cond2))
-      && XEXP (cond1, 0) == XEXP (cond2, 0)
+      && rtx_equal_p (XEXP (cond1, 0), XEXP (cond2, 0))
       && XEXP (cond1, 1) == XEXP (cond2, 1))
     return 1;
   return 0;
@@ -2722,14 +2722,13 @@
 
           if (sched_deps_info->compute_jump_reg_dependencies)
             {
-              regset_head tmp_uses, tmp_sets;
-              INIT_REG_SET (&tmp_uses);
-              INIT_REG_SET (&tmp_sets);
+              regset_head tmp;
+              INIT_REG_SET (&tmp);
 
-              (*sched_deps_info->compute_jump_reg_dependencies)
-                (insn, &deps->reg_conditional_sets, &tmp_uses, &tmp_sets);
+              (*sched_deps_info->compute_jump_reg_dependencies) (insn, &tmp);
+
               /* Make latency of jump equal to 0 by using anti-dependence.  */
-              EXECUTE_IF_SET_IN_REG_SET (&tmp_uses, 0, i, rsi)
+              EXECUTE_IF_SET_IN_REG_SET (&tmp, 0, i, rsi)
                 {
                   struct deps_reg *reg_last = &deps->reg_last[i];
                   add_dependence_list (insn, reg_last->sets, 0, REG_DEP_ANTI);
@@ -2744,10 +2743,8 @@
                       reg_last->uses = alloc_INSN_LIST (insn, reg_last->uses);
                     }
                 }
-              IOR_REG_SET (reg_pending_sets, &tmp_sets);
 
-              CLEAR_REG_SET (&tmp_uses);
-              CLEAR_REG_SET (&tmp_sets);
+              CLEAR_REG_SET (&tmp);
             }
 
 	  /* All memory writes and volatile reads must happen before the
@@ -2920,10 +2917,7 @@
 	      add_dependence_list (insn, reg_last->uses, 0, REG_DEP_ANTI);
 
 	      if (!deps->readonly)
-		{
-		  reg_last->sets = alloc_INSN_LIST (insn, reg_last->sets);
-		  SET_REGNO_REG_SET (&deps->reg_conditional_sets, i);
-		}
+		reg_last->sets = alloc_INSN_LIST (insn, reg_last->sets);
 	    }
 	}
       else
@@ -2985,7 +2979,6 @@
 		  reg_last->sets = alloc_INSN_LIST (insn, reg_last->sets);
 		  reg_last->uses_length = 0;
 		  reg_last->clobbers_length = 0;
-		  CLEAR_REGNO_REG_SET (&deps->reg_conditional_sets, i);
 		}
 	    }
 	}
@@ -3083,8 +3076,6 @@
                              && sel_insn_is_speculation_check (insn)))
 	flush_pending_lists (deps, insn, true, true);
 
-      if (!deps->readonly)
-        CLEAR_REG_SET (&deps->reg_conditional_sets);
       reg_pending_barrier = NOT_A_BARRIER;
     }
 
@@ -3521,7 +3512,6 @@
   else
     deps->reg_last = XCNEWVEC (struct deps_reg, max_reg);
   INIT_REG_SET (&deps->reg_last_in_use);
-  INIT_REG_SET (&deps->reg_conditional_sets);
 
   deps->pending_read_insns = 0;
   deps->pending_read_mems = 0;
@@ -3590,7 +3580,6 @@
 	free_INSN_LIST_list (&reg_last->clobbers);
     }
   CLEAR_REG_SET (&deps->reg_last_in_use);
-  CLEAR_REG_SET (&deps->reg_conditional_sets);
 
   /* As we initialize reg_last lazily, it is possible that we didn't allocate
      it at all.  */
@@ -3600,8 +3589,7 @@
   deps = NULL;
 }
 
-/* Remove INSN from dependence contexts DEPS.  Caution: reg_conditional_sets
-   is not handled.  */
+/* Remove INSN from dependence contexts DEPS.  */
 void
 remove_from_deps (struct deps_desc *deps, rtx insn)
 {
Index: gcc/sched-int.h
===================================================================
--- gcc/sched-int.h	(revision 176195)
+++ gcc/sched-int.h	(working copy)
@@ -173,7 +173,7 @@
 
 extern int max_issue (struct ready_list *, int, state_t, bool, int *);
 
-extern void ebb_compute_jump_reg_dependencies (rtx, regset, regset, regset);
+extern void ebb_compute_jump_reg_dependencies (rtx, regset);
 
 extern edge find_fallthru_edge_from (basic_block);
 
@@ -511,9 +511,6 @@
      in reg_last[N].{uses,sets,clobbers}.  */
   regset_head reg_last_in_use;
 
-  /* Element N is set for each register that is conditionally set.  */
-  regset_head reg_conditional_sets;
-
   /* Shows the last value of reg_pending_barrier associated with the insn.  */
   enum reg_pending_barrier_mode last_reg_pending_barrier;
 
@@ -1117,7 +1114,7 @@
   /* Called when computing dependencies for a JUMP_INSN.  This function
      should store the set of registers that must be considered as set by
      the jump in the regset.  */
-  void (*compute_jump_reg_dependencies) (rtx, regset, regset, regset);
+  void (*compute_jump_reg_dependencies) (rtx, regset);
 
   /* Start analyzing insn.  */
   void (*start_insn) (rtx);
Index: gcc/sched-rgn.c
===================================================================
--- gcc/sched-rgn.c	(revision 176195)
+++ gcc/sched-rgn.c	(working copy)
@@ -2062,7 +2062,7 @@
 static int schedule_more_p (void);
 static const char *rgn_print_insn (const_rtx, int);
 static int rgn_rank (rtx, rtx);
-static void compute_jump_reg_dependencies (rtx, regset, regset, regset);
+static void compute_jump_reg_dependencies (rtx, regset);
 
 /* Functions for speculative scheduling.  */
 static void rgn_add_remove_insn (rtx, int);
@@ -2295,16 +2295,12 @@
   return BLOCK_TO_BB (BLOCK_NUM (next)) == BLOCK_TO_BB (BLOCK_NUM (insn));
 }
 
-/* INSN is a JUMP_INSN, COND_SET is the set of registers that are
-   conditionally set before INSN.  Store the set of registers that
-   must be considered as used by this jump in USED and that of
-   registers that must be considered as set in SET.  */
+/* INSN is a JUMP_INSN.  Store the set of registers that must be
+   considered as used by this jump in USED.  */
 
 static void
 compute_jump_reg_dependencies (rtx insn ATTRIBUTE_UNUSED,
-			       regset cond_exec ATTRIBUTE_UNUSED,
-			       regset used ATTRIBUTE_UNUSED,
-			       regset set ATTRIBUTE_UNUSED)
+			       regset used ATTRIBUTE_UNUSED)
 {
   /* Nothing to do here, since we postprocess jumps in
      add_branch_dependences.  */
Index: gcc/config/ia64/ia64.c
===================================================================
--- gcc/config/ia64/ia64.c	(revision 176195)
+++ gcc/config/ia64/ia64.c	(working copy)
@@ -1047,7 +1047,7 @@
       tmp = gen_rtx_PLUS (Pmode, tmp, pic_offset_table_rtx);
       emit_insn (gen_rtx_SET (VOIDmode, dest, tmp));
 
-      tmp = gen_rtx_LO_SUM (Pmode, dest, src);
+      tmp = gen_rtx_LO_SUM (Pmode, gen_rtx_MEM (Pmode, dest), src);
       emit_insn (gen_rtx_SET (VOIDmode, dest, tmp));
 
       if (addend)
Index: gcc/config/ia64/ia64.md
===================================================================
--- gcc/config/ia64/ia64.md	(revision 176195)
+++ gcc/config/ia64/ia64.md	(working copy)
@@ -777,7 +777,7 @@
 
 (define_insn "*load_symptr_low"
   [(set (match_operand:DI 0 "register_operand" "=r")
-	(lo_sum:DI (match_operand:DI 1 "register_operand" "r")
+	(lo_sum:DI (mem:DI (match_operand:DI 1 "register_operand" "r"))
 		   (match_operand 2 "got_symbolic_operand" "s")))]
   "reload_completed"
 {

Re: Correct fix for scheduler bug PR11320

[Andrey Belevantsev]

Hello Bernd,

FWIW, we have discussed your change with Alexander and we think you are 
right about the scheduler changes.  One question is:

On 14.07.2011 14:03, Bernd Schmidt wrote:
> --- gcc/sched-deps.c	(revision 176195)
> +++ gcc/sched-deps.c	(working copy)
> @@ -568,7 +568,7 @@
>  	  (rev1==rev2
>  	  ? reversed_comparison_code (cond2, NULL)
>  	  : GET_CODE (cond2))
> -      && XEXP (cond1, 0) == XEXP (cond2, 0)
> +      && rtx_equal_p (XEXP (cond1, 0), XEXP (cond2, 0))
>        && XEXP (cond1, 1) == XEXP (cond2, 1))
>      return 1;
>    return 0;

this hunk from conditions_mutex_p seems to be unrelated?

Andrey

Re: Correct fix for scheduler bug PR11320

[Bernd Schmidt]

On 07/14/11 13:20, Andrey Belevantsev wrote:
> Hello Bernd,
> 
> FWIW, we have discussed your change with Alexander and we think you are
> right about the scheduler changes.  One question is:
> 
> On 14.07.2011 14:03, Bernd Schmidt wrote:
>> --- gcc/sched-deps.c    (revision 176195)
>> +++ gcc/sched-deps.c    (working copy)
>> @@ -568,7 +568,7 @@
>>        (rev1==rev2
>>        ? reversed_comparison_code (cond2, NULL)
>>        : GET_CODE (cond2))
>> -      && XEXP (cond1, 0) == XEXP (cond2, 0)
>> +      && rtx_equal_p (XEXP (cond1, 0), XEXP (cond2, 0))
>>        && XEXP (cond1, 1) == XEXP (cond2, 1))
>>      return 1;
>>    return 0;
> 
> this hunk from conditions_mutex_p seems to be unrelated?

Oh yes, sorry about that. That was approved a while ago and I haven't
gotten around to checking it in.


Bernd

Re: Correct fix for scheduler bug PR11320

[Eric Botcazou]

> The real problem here is that the ia64 backend lies to the rest of the
> compiler; it produces a load instruction without showing a MEM anywhere
> in the instruction pattern. Hence, the following patch, which reverts
> the bogus scheduler changes and adds a MEM to a pattern in ia64.md.

This is probably the root cause of the problem, indeed.  But you don't revert 
everything so, if this isn't an oversight, then the ChangeLog is incorrect.
And there is another change in sched-deps.c not mentioned in the ChangeLog.

-- 
Eric Botcazou

Re: Correct fix for scheduler bug PR11320

[Bernd Schmidt]

On 07/14/11 13:57, Eric Botcazou wrote:
>> The real problem here is that the ia64 backend lies to the rest of the
>> compiler; it produces a load instruction without showing a MEM anywhere
>> in the instruction pattern. Hence, the following patch, which reverts
>> the bogus scheduler changes and adds a MEM to a pattern in ia64.md.
> 
> This is probably the root cause of the problem, indeed.  But you don't revert 
> everything so, if this isn't an oversight, then the ChangeLog is incorrect.
> And there is another change in sched-deps.c not mentioned in the ChangeLog.

Well, the actual code has completely changed in the meantime. All the
hunks of the original patch failed :) I can write a new ChangeLog entry
if that seems important.

Any particular bits you still see that don't get reverted with this patch?


Bernd

Re: Correct fix for scheduler bug PR11320

[Eric Botcazou]

> Any particular bits you still see that don't get reverted with this patch?

The ebb_compute_jump_reg_dependencies changes.  The original patch has:

	* sched-ebb.c (compute_jump_reg_dependencies): New prototype.
	Mark registers live on entry of the fallthrough block and conditionally
	set as set by the jump. Mark registers live on entry of non-fallthrough
	blocks as used by the jump.

but you're reverting only:

	* sched-ebb.c (compute_jump_reg_dependencies): New prototype.
	Mark registers live on entry of the fallthrough block and conditionally
	set as set by the jump.

-- 
Eric Botcazou

Re: Correct fix for scheduler bug PR11320

[Bernd Schmidt]

On 07/14/11 14:18, Eric Botcazou wrote:
>> Any particular bits you still see that don't get reverted with this patch?
> 
> The ebb_compute_jump_reg_dependencies changes.  The original patch has:
> 
> 	* sched-ebb.c (compute_jump_reg_dependencies): New prototype.
> 	Mark registers live on entry of the fallthrough block and conditionally
> 	set as set by the jump. Mark registers live on entry of non-fallthrough
> 	blocks as used by the jump.
> 
> but you're reverting only:
> 
> 	* sched-ebb.c (compute_jump_reg_dependencies): New prototype.
> 	Mark registers live on entry of the fallthrough block and conditionally
> 	set as set by the jump.
> 

??? Original code:

   basic_block b = BLOCK_FOR_INSN (insn);
    edge e;
    for (e = b->succ; e; e = e->succ_next)
!     if ((e->flags & EDGE_FALLTHRU) == 0)
!       {
! 	bitmap_operation (set, set, e->dest->global_live_at_start,
! 			  BITMAP_IOR);
!       }
  }

Code after the revert:

   FOR_EACH_EDGE (e, ei, b->succs)
+    if ((e->flags & EDGE_FALLTHRU) == 0)
       bitmap_ior_into (used, df_get_live_in (e->dest));

As far as I can tell these are identical, modulo the change in variable
name ("set" -> "used" which seems like a better name).


Bernd

Re: Correct fix for scheduler bug PR11320

[Eric Botcazou]

> ??? Original code:
>
>    basic_block b = BLOCK_FOR_INSN (insn);
>     edge e;
>     for (e = b->succ; e; e = e->succ_next)
> !     if ((e->flags & EDGE_FALLTHRU) == 0)
> !       {
> ! 	bitmap_operation (set, set, e->dest->global_live_at_start,
> ! 			  BITMAP_IOR);
> !       }
>   }
>
> Code after the revert:
>
>    FOR_EACH_EDGE (e, ei, b->succs)
> +    if ((e->flags & EDGE_FALLTHRU) == 0)
>        bitmap_ior_into (used, df_get_live_in (e->dest));
>
> As far as I can tell these are identical, modulo the change in variable
> name ("set" -> "used" which seems like a better name).

Yes, the code does the same thing, but the original patch did clear up the 
confusion set/use in sched_analyze_insn and compute_jump_reg_dependencies,
in particular in the comment of the latter.  But OK, never mind.

-- 
Eric Botcazou

Re: Correct fix for scheduler bug PR11320

[Richard Henderson]

On 07/14/2011 03:03 AM, Bernd Schmidt wrote:
> +++ gcc/config/ia64/ia64.c	(working copy)
> @@ -1047,7 +1047,7 @@
>        tmp = gen_rtx_PLUS (Pmode, tmp, pic_offset_table_rtx);
>        emit_insn (gen_rtx_SET (VOIDmode, dest, tmp));
>  
> -      tmp = gen_rtx_LO_SUM (Pmode, dest, src);
> +      tmp = gen_rtx_LO_SUM (Pmode, gen_rtx_MEM (Pmode, dest), src);

And the bug stems from ... what? 

Is this bug still fixed if you change this to gen_const_mem?

This is a load from the .got.  It's constant memory, and it's
always present.  There's nowhere in the function that we cannot
move this load (assuming the address is computed properly) 
where this load will fail.

It's difficult to tell if your raw gen_rtx_MEM with no aliasing
info doesn't just paper over a problem by preventing it from
being moved.


r~

Re: Correct fix for scheduler bug PR11320

[Bernd Schmidt]

On 07/14/11 18:03, Richard Henderson wrote:
> On 07/14/2011 03:03 AM, Bernd Schmidt wrote:
>> +++ gcc/config/ia64/ia64.c	(working copy)
>> @@ -1047,7 +1047,7 @@
>>        tmp = gen_rtx_PLUS (Pmode, tmp, pic_offset_table_rtx);
>>        emit_insn (gen_rtx_SET (VOIDmode, dest, tmp));
>>  
>> -      tmp = gen_rtx_LO_SUM (Pmode, dest, src);
>> +      tmp = gen_rtx_LO_SUM (Pmode, gen_rtx_MEM (Pmode, dest), src);
> 
> And the bug stems from ... what? 
> 
> Is this bug still fixed if you change this to gen_const_mem?

It should be. Testing this isn't straightforward bit tricky since the
original bug is in gcc-3.3 which doesn't have gen_const_mem, and current
mainline with just the scheduler patch removed doesn't reproduce it with
the testcase.

In mainline, gen_const_mem sets MEM_NOTRAP_P, but it looks like we
handle that correctly in may_trap_p:

      if (/* MEM_NOTRAP_P only relates to the actual position of the memory
             reference; moving it out of context such as when moving code
             when optimizing, might cause its address to become invalid.  */
          code_changed
          || !MEM_NOTRAP_P (x))

and sched_deps uses rtx_addr_can_trap anyway.

> This is a load from the .got.

Yes, but not using the fixed got pointer in r1, but a random other
register which can have different values in the function.

> It's difficult to tell if your raw gen_rtx_MEM with no aliasing
> info doesn't just paper over a problem by preventing it from
> being moved.

The problem isn't about memory aliasing, it's about the pointer register
being clobbered.


Bernd

Re: Correct fix for scheduler bug PR11320

[Bernd Schmidt]

On 07/14/11 18:19, Bernd Schmidt wrote:
> On 07/14/11 18:03, Richard Henderson wrote:
>> On 07/14/2011 03:03 AM, Bernd Schmidt wrote:
>>> +++ gcc/config/ia64/ia64.c	(working copy)
>>> @@ -1047,7 +1047,7 @@
>>>        tmp = gen_rtx_PLUS (Pmode, tmp, pic_offset_table_rtx);
>>>        emit_insn (gen_rtx_SET (VOIDmode, dest, tmp));
>>>  
>>> -      tmp = gen_rtx_LO_SUM (Pmode, dest, src);
>>> +      tmp = gen_rtx_LO_SUM (Pmode, gen_rtx_MEM (Pmode, dest), src);
>>
>> And the bug stems from ... what? 
>>
>> Is this bug still fixed if you change this to gen_const_mem?
> 
> It should be. Testing this isn't straightforward bit tricky since the
> original bug is in gcc-3.3 which doesn't have gen_const_mem, and current
> mainline with just the scheduler patch removed doesn't reproduce it with
> the testcase.

Ok, with gen_const_mem hacked into gcc-3.3 (minus setting MEM_READONLY_P
which doesn't exist in that tree) the load stays behind the branch where
it should be.


Bernd

Re: Correct fix for scheduler bug PR11320

[Richard Henderson]

On 07/14/2011 09:23 AM, Bernd Schmidt wrote:
> Ok, with gen_const_mem hacked into gcc-3.3 (minus setting MEM_READONLY_P
> which doesn't exist in that tree) the load stays behind the branch where
> it should be.
> 

RTX_UNCHANGING_P was the bit back then, I believe.


r~

Re: Correct fix for scheduler bug PR11320

[Bernd Schmidt]

[-- Attachment #1: Type: text/plain, Size: 1063 bytes --]

On 07/14/11 18:29, Richard Henderson wrote:
> On 07/14/2011 09:23 AM, Bernd Schmidt wrote:
>> Ok, with gen_const_mem hacked into gcc-3.3 (minus setting MEM_READONLY_P
>> which doesn't exist in that tree) the load stays behind the branch where
>> it should be.
>>
> 
> RTX_UNCHANGING_P was the bit back then, I believe.

Still ok with that also set:
        addl r14 = @ltoff(ap_standalone#), r1
        ;;
        .mii
        ld8 r15 = [r14]
        addl r14 = @ltoff(.LC2), r1
        ;;
        (p7) addl r14 = 1, r0
        ;;
        .mib
        (p7) st4 [r15] = r14
        nop.i 0
        (p7) br.cond.dptk .L5
        .mib
        ld8 r14 = [r14]

And not ok if the MEM isn't exposed in RTL:
        addl r14 = @ltoff(ap_standalone#), r1
        ;;
        .mii
        ld8 r15 = [r14]
        addl r14 = @ltoff(.LC2), r1
        ;;
        (p7) addl r14 = 1, r0
        ;;
        .mii
        (p7) st4 [r15] = r14
        nop.i 0
        nop.i 0
        .mbb
        ld8 r14 = [r14]
        (p7) br.cond.dptk .L5

I'm attaching the 3.3 patch.


Bernd


[-- Attachment #2: 3.3-ia64.diff --]
[-- Type: text/plain, Size: 9738 bytes --]

Index: ../../branches/gcc-3_3-branch/gcc/sched-ebb.c
===================================================================
--- ../../branches/gcc-3_3-branch/gcc/sched-ebb.c	(revision 175226)
+++ ../../branches/gcc-3_3-branch/gcc/sched-ebb.c	(working copy)
@@ -52,8 +52,7 @@ static int schedule_more_p PARAMS ((void
 static const char *ebb_print_insn PARAMS ((rtx, int));
 static int rank PARAMS ((rtx, rtx));
 static int contributes_to_priority PARAMS ((rtx, rtx));
-static void compute_jump_reg_dependencies PARAMS ((rtx, regset, regset,
-						   regset));
+static void compute_jump_reg_dependencies PARAMS ((rtx, regset));
 static void schedule_ebb PARAMS ((rtx, rtx));
 
 /* Return nonzero if there are more insns that should be scheduled.  */
@@ -161,30 +160,22 @@ contributes_to_priority (next, insn)
   return 1;
 }
 
-/* INSN is a JUMP_INSN, COND_SET is the set of registers that are
-   conditionally set before INSN.  Store the set of registers that
-   must be considered as used by this jump in USED and that of
-   registers that must be considered as set in SET.  */
+/* INSN is a JUMP_INSN.  Store the set of registers that must be considered
+   to be set by this jump in SET.  */
 
 static void
-compute_jump_reg_dependencies (insn, cond_set, used, set)
+compute_jump_reg_dependencies (insn, set)
      rtx insn;
-     regset cond_set, used, set;
+     regset set;
 {
   basic_block b = BLOCK_FOR_INSN (insn);
   edge e;
   for (e = b->succ; e; e = e->succ_next)
-    if (e->flags & EDGE_FALLTHRU)
-      /* The jump may be a by-product of a branch that has been merged
-	 in the main codepath after being conditionalized.  Therefore
-	 it may guard the fallthrough block from using a value that has
-	 conditionally overwritten that of the main codepath.  So we
-	 consider that it restores the value of the main codepath.  */
-      bitmap_operation (set, e->dest->global_live_at_start, cond_set,
-			BITMAP_AND);
-    else
-      bitmap_operation (used, used, e->dest->global_live_at_start,
-			BITMAP_IOR);
+    if ((e->flags & EDGE_FALLTHRU) == 0)
+      {
+	bitmap_operation (set, set, e->dest->global_live_at_start,
+			  BITMAP_IOR);
+      }
 }
 
 /* Used in schedule_insns to initialize current_sched_info for scheduling
Index: ../../branches/gcc-3_3-branch/gcc/emit-rtl.c
===================================================================
--- ../../branches/gcc-3_3-branch/gcc/emit-rtl.c	(revision 175226)
+++ ../../branches/gcc-3_3-branch/gcc/emit-rtl.c	(working copy)
@@ -192,6 +192,18 @@ static tree component_ref_for_mem_expr	P
 static rtx gen_const_vector_0		PARAMS ((enum machine_mode));
 static void copy_rtx_if_shared_1	PARAMS ((rtx *orig));
 
+/* Generate a memory referring to non-trapping constant memory.  */
+
+rtx
+gen_const_mem (enum machine_mode mode, rtx addr)
+{
+  rtx mem = gen_rtx_MEM (mode, addr);
+  RTX_UNCHANGING_P (mem) = 1;
+  MEM_NOTRAP_P (mem) = 1;
+  return mem;
+}
+
+
 /* Probability of the conditional branch currently proceeded by try_split.
    Set to -1 otherwise.  */
 int split_branch_probability = -1;
Index: ../../branches/gcc-3_3-branch/gcc/sched-deps.c
===================================================================
--- ../../branches/gcc-3_3-branch/gcc/sched-deps.c	(revision 175226)
+++ ../../branches/gcc-3_3-branch/gcc/sched-deps.c	(working copy)
@@ -981,17 +981,12 @@ sched_analyze_insn (deps, x, insn, loop_
       else
 	{
 	  rtx pending, pending_mem;
-	  regset_head tmp_uses, tmp_sets;
-	  INIT_REG_SET (&tmp_uses);
-	  INIT_REG_SET (&tmp_sets);
-
-	  (*current_sched_info->compute_jump_reg_dependencies)
-	    (insn, &deps->reg_conditional_sets, &tmp_uses, &tmp_sets);
-	  IOR_REG_SET (reg_pending_uses, &tmp_uses);
-	  IOR_REG_SET (reg_pending_sets, &tmp_sets);
+	  regset_head tmp;
+	  INIT_REG_SET (&tmp);
 
-	  CLEAR_REG_SET (&tmp_uses);
-	  CLEAR_REG_SET (&tmp_sets);
+	  (*current_sched_info->compute_jump_reg_dependencies) (insn, &tmp);
+	  IOR_REG_SET (reg_pending_uses, &tmp);
+	  CLEAR_REG_SET (&tmp);
 
 	  /* All memory writes and volatile reads must happen before the
 	     jump.  Non-volatile reads must happen before the jump iff
@@ -1088,7 +1083,6 @@ sched_analyze_insn (deps, x, insn, loop_
 	}
 
       flush_pending_lists (deps, insn, true, true);
-      CLEAR_REG_SET (&deps->reg_conditional_sets);
       reg_pending_barrier = false;
     }
   else
@@ -1120,7 +1114,6 @@ sched_analyze_insn (deps, x, insn, loop_
 	      add_dependence_list (insn, reg_last->clobbers, REG_DEP_OUTPUT);
 	      add_dependence_list (insn, reg_last->uses, REG_DEP_ANTI);
 	      reg_last->sets = alloc_INSN_LIST (insn, reg_last->sets);
-	      SET_REGNO_REG_SET (&deps->reg_conditional_sets, i);
 	    });
 	}
       else
@@ -1169,7 +1162,6 @@ sched_analyze_insn (deps, x, insn, loop_
 	      reg_last->sets = alloc_INSN_LIST (insn, reg_last->sets);
 	      reg_last->uses_length = 0;
 	      reg_last->clobbers_length = 0;
-	      CLEAR_REGNO_REG_SET (&deps->reg_conditional_sets, i);
 	    });
 	}
 
@@ -1496,7 +1488,6 @@ init_deps (deps)
   deps->reg_last = (struct deps_reg *)
     xcalloc (max_reg, sizeof (struct deps_reg));
   INIT_REG_SET (&deps->reg_last_in_use);
-  INIT_REG_SET (&deps->reg_conditional_sets);
 
   deps->pending_read_insns = 0;
   deps->pending_read_mems = 0;
@@ -1539,7 +1530,6 @@ free_deps (deps)
 	free_INSN_LIST_list (&reg_last->clobbers);
     });
   CLEAR_REG_SET (&deps->reg_last_in_use);
-  CLEAR_REG_SET (&deps->reg_conditional_sets);
 
   free (deps->reg_last);
 }
Index: ../../branches/gcc-3_3-branch/gcc/sched-int.h
===================================================================
--- ../../branches/gcc-3_3-branch/gcc/sched-int.h	(revision 175226)
+++ ../../branches/gcc-3_3-branch/gcc/sched-int.h	(working copy)
@@ -112,9 +112,6 @@ struct deps
   /* Element N is set for each register that has any nonzero element
      in reg_last[N].{uses,sets,clobbers}.  */
   regset_head reg_last_in_use;
-
-  /* Element N is set for each register that is conditionally set.  */
-  regset_head reg_conditional_sets;
 };
 
 /* This structure holds some state of the current scheduling pass, and
@@ -149,9 +146,9 @@ struct sched_info
      calculations.  */
   int (*contributes_to_priority) PARAMS ((rtx, rtx));
   /* Called when computing dependencies for a JUMP_INSN.  This function
-     should store the set of registers that must be considered as used
-     and the set of registers that must be considered as set by the jump.  */
-  void (*compute_jump_reg_dependencies) PARAMS ((rtx, regset, regset, regset));
+     should store the set of registers that must be considered as set by
+     the jump in the regset.  */
+  void (*compute_jump_reg_dependencies) PARAMS ((rtx, regset));
 
   /* The boundaries of the set of insns to be scheduled.  */
   rtx prev_head, next_tail;
Index: ../../branches/gcc-3_3-branch/gcc/sched-rgn.c
===================================================================
--- ../../branches/gcc-3_3-branch/gcc/sched-rgn.c	(revision 175226)
+++ ../../branches/gcc-3_3-branch/gcc/sched-rgn.c	(working copy)
@@ -1968,8 +1968,7 @@ static int schedule_more_p PARAMS ((void
 static const char *rgn_print_insn PARAMS ((rtx, int));
 static int rgn_rank PARAMS ((rtx, rtx));
 static int contributes_to_priority PARAMS ((rtx, rtx));
-static void compute_jump_reg_dependencies PARAMS ((rtx, regset, regset,
-						   regset));
+static void compute_jump_reg_dependencies PARAMS ((rtx, regset));
 
 /* Return nonzero if there are more insns that should be scheduled.  */
 
@@ -2256,16 +2255,12 @@ contributes_to_priority (next, insn)
   return BLOCK_NUM (next) == BLOCK_NUM (insn);
 }
 
-/* INSN is a JUMP_INSN, COND_SET is the set of registers that are
-   conditionally set before INSN.  Store the set of registers that
-   must be considered as used by this jump in USED and that of
-   registers that must be considered as set in SET.  */
+/* INSN is a JUMP_INSN.  Store the set of registers that must be considered
+   to be set by this jump in SET.  */
 
 static void
-compute_jump_reg_dependencies (insn, cond_set, used, set)
+compute_jump_reg_dependencies (insn, set)
      rtx insn ATTRIBUTE_UNUSED;
-     regset cond_set ATTRIBUTE_UNUSED;
-     regset used ATTRIBUTE_UNUSED;
      regset set ATTRIBUTE_UNUSED;
 {
   /* Nothing to do here, since we postprocess jumps in
Index: ../../branches/gcc-3_3-branch/gcc/config/ia64/ia64.md
===================================================================
--- ../../branches/gcc-3_3-branch/gcc/config/ia64/ia64.md	(revision 175226)
+++ ../../branches/gcc-3_3-branch/gcc/config/ia64/ia64.md	(working copy)
@@ -534,13 +534,19 @@ (define_expand "load_symptr"
 	(plus:DI (high:DI (match_operand:DI 1 "got_symbolic_operand" ""))
 		 (match_dup 3)))
    (set (match_operand:DI 0 "register_operand" "")
-	(lo_sum:DI (match_dup 2) (match_dup 1)))]
+	(lo_sum:DI (mem:DI (match_dup 2)) (match_dup 1)))]
   ""
 {
   operands[3] = pic_offset_table_rtx;
+  emit_insn (gen_load_symptr_high (operands[2], operands[1], operands[3]));
+  emit_insn (gen_rtx_SET (VOIDmode, operands[0],
+			  gen_rtx_LO_SUM (DImode, gen_const_mem (DImode,
+								 operands[2]),
+					  operands[1])));
+  DONE;
 })
 
-(define_insn "*load_symptr_high"
+(define_insn "load_symptr_high"
   [(set (match_operand:DI 0 "register_operand" "=r")
 	(plus:DI (high:DI (match_operand 1 "got_symbolic_operand" "s"))
 		 (match_operand:DI 2 "register_operand" "a")))]
@@ -555,7 +561,7 @@ (define_insn "*load_symptr_high"
 
 (define_insn "*load_symptr_low"
   [(set (match_operand:DI 0 "register_operand" "=r")
-	(lo_sum:DI (match_operand:DI 1 "register_operand" "r")
+	(lo_sum:DI (mem:DI (match_operand:DI 1 "register_operand" "r"))
 		   (match_operand 2 "got_symbolic_operand" "s")))]
   ""
 {

Re: Correct fix for scheduler bug PR11320

[Richard Henderson]

On 07/14/2011 09:19 AM, Bernd Schmidt wrote:
> Yes, but not using the fixed got pointer in r1, but a random other
> register which can have different values in the function.

Oh, I think I see.

So if this really had been a PLUS, as implied by the LO_SUM,
we would have had garbage input, produced garbage output, but
(eventually) ignored the result.

But since this really is a load from memory, the garbage
input is immediately fatal.

Have I got that right?

If so, the patch with the use of gen_const_mem is ok.

It does raise the question of whether we ought to completely
change the way we represent the pairing of LTOFFX/LDXMOV
relocations.


r~

Re: Correct fix for scheduler bug PR11320

[Bernd Schmidt]

On 07/14/11 18:39, Richard Henderson wrote:
> On 07/14/2011 09:19 AM, Bernd Schmidt wrote:
>> Yes, but not using the fixed got pointer in r1, but a random other
>> register which can have different values in the function.
> 
> Oh, I think I see.
> 
> So if this really had been a PLUS, as implied by the LO_SUM,
> we would have had garbage input, produced garbage output, but
> (eventually) ignored the result.
> 
> But since this really is a load from memory, the garbage
> input is immediately fatal.
> 
> Have I got that right?

This is correct.

> If so, the patch with the use of gen_const_mem is ok.

Will commit.

(Although now I wonder if we could instead use one of the speculative
load instructions? There's one that sets the NaT bit if the load would
fault, isn't there? It's been so long I can't remember.)

> It does raise the question of whether we ought to completely
> change the way we represent the pairing of LTOFFX/LDXMOV
> relocations.

This I can't answer since I don't know the definition of these.


Bernd

Re: Correct fix for scheduler bug PR11320

[Richard Henderson]

On 07/14/2011 09:43 AM, Bernd Schmidt wrote:
> (Although now I wonder if we could instead use one of the speculative
> load instructions? There's one that sets the NaT bit if the load would
> fault, isn't there? It's been so long I can't remember.)

We could, but we also have to insert a check load to match.
It gets very difficult to tell when it's worth it.

Your example

        (p7) br.cond.dptk .L5
        ld8 r15 = [r14]

becomes

        ld8.sa r15 = [r14]		// speculative advanced load
        (p7) br.cond.dptk .L5
	ld8.c.clr r15 = [r14]		// checked load (clear ALAT)

Note that the speculative load can arbitrarily fail (e.g. tlb miss)
and that the checked load can also arbitrarily re-issue the load.

Note that one can't split "ld8 r14 = [r14]" without additional
register allocation, because the address needs to remain live
until the check.

>> It does raise the question of whether we ought to completely
>> change the way we represent the pairing of LTOFFX/LDXMOV
>> relocations.
> 
> This I can't answer since I don't know the definition of these.

These are markers for linker relaxation.  If the symbol is
within range,

	addl	x = @ltoffx(foo), gp
	...
	ld8.mov	y = [x], foo	// .mov adds the LDXMOV reloc vs foo to ld8 insn

will be relaxed to

	nop
	...
	addl	y = @gprel(foo), gp

The constraint in using the relocations is that every ldxmov
insn must be fed by an ltoffx reloc with the same symbol, and
that an ltoffx reloc cannot feed any other insn.  That allows
us, at link time, to not consider data flow, merely assert
that if foo is relaxed anywhere, it's relaxed everywhere.


r~

Re: Correct fix for scheduler bug PR11320

[Andreas Schwab]

I'm now seeing this ICE:

/bin/sh ./libtool --tag=GCJ   --mode=compile /usr/local/gcc/gcc-20110719/Build/./gcc/gcj -B/usr/local/gcc/gcc-20110719/Build/ia64-suse-linux/libjava/ -B/usr/local/gcc/gcc-20110719/Build/./gcc/ -B/usr/ia64-suse-linux/bin/ -B/usr/ia64-suse-linux/lib/ -isystem /usr/ia64-suse-linux/include -isystem /usr/ia64-suse-linux/sys-include    -funwind-tables -fclasspath= -fbootclasspath=../../../libjava/classpath/lib --encoding=UTF-8 -Wno-deprecated -fbootstrap-classes -g -O2  -c -o java/lang/ref.lo -fsource-filename=/usr/local/gcc/gcc-20110719/Build/ia64-suse-linux/libjava/classpath/lib/classes -MT java/lang/ref.lo -MD -MP -MF java/lang/ref.deps @java/lang/ref.list
libtool: compile:  /usr/local/gcc/gcc-20110719/Build/./gcc/gcj -B/usr/local/gcc/gcc-20110719/Build/ia64-suse-linux/libjava/ -B/usr/local/gcc/gcc-20110719/Build/./gcc/ -B/usr/ia64-suse-linux/bin/ -B/usr/ia64-suse-linux/lib/ -isystem /usr/ia64-suse-linux/include -isystem /usr/ia64-suse-linux/sys-include -funwind-tables -fclasspath= -fbootclasspath=../../../libjava/classpath/lib --encoding=UTF-8 -Wno-deprecated -fbootstrap-classes -g -O2 -c -fsource-filename=/usr/local/gcc/gcc-20110719/Build/ia64-suse-linux/libjava/classpath/lib/classes -MT java/lang/ref.lo -MD -MP -MF java/lang/ref.deps @java/lang/ref.list  -fPIC -o java/lang/.libs/ref.o
/usr/local/gcc/gcc-20110719/libjava/classpath/java/lang/ref/ReferenceQueue.java: In class 'java.lang.ref.ReferenceQueue':
/usr/local/gcc/gcc-20110719/libjava/classpath/java/lang/ref/ReferenceQueue.java: In method 'java.lang.ref.ReferenceQueue.enqueue(java.lang.ref.Reference)':
In file included from /usr/local/gcc/gcc-20110719/libjava/java/lang/ref/Reference.java:202:0,
                 from /usr/local/gcc/gcc-20110719/libjava/classpath/java/lang/ref/PhantomReference.java:75,
                 from <built-in>:6:
/usr/local/gcc/gcc-20110719/libjava/classpath/java/lang/ref/ReferenceQueue.java:97:0: internal compiler error: in advance_target_bb, at sched-ebb.c:691
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.
make[3]: *** [java/lang/ref.lo] Error 1

Andreas.

-- 
Andreas Schwab, schwab@redhat.com
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."

Re: Correct fix for scheduler bug PR11320

[Bernd Schmidt]

On 07/19/11 14:25, Andreas Schwab wrote:
> I'm now seeing this ICE:
> 
> /bin/sh ./libtool --tag=GCJ   --mode=compile /usr/local/gcc/gcc-20110719/Build/./gcc/gcj -B/usr/local/gcc/gcc-20110719/Build/ia64-suse-linux/libjava/ -B/usr/local/gcc/gcc-20110719/Build/./gcc/ -B/usr/ia64-suse-linux/bin/ -B/usr/ia64-suse-linux/lib/ -isystem /usr/ia64-suse-linux/include -isystem /usr/ia64-suse-linux/sys-include    -funwind-tables -fclasspath= -fbootclasspath=../../../libjava/classpath/lib --encoding=UTF-8 -Wno-deprecated -fbootstrap-classes -g -O2  -c -o java/lang/ref.lo -fsource-filename=/usr/local/gcc/gcc-20110719/Build/ia64-suse-linux/libjava/classpath/lib/classes -MT java/lang/ref.lo -MD -MP -MF java/lang/ref.deps @java/lang/ref.list
> libtool: compile:  /usr/local/gcc/gcc-20110719/Build/./gcc/gcj -B/usr/local/gcc/gcc-20110719/Build/ia64-suse-linux/libjava/ -B/usr/local/gcc/gcc-20110719/Build/./gcc/ -B/usr/ia64-suse-linux/bin/ -B/usr/ia64-suse-linux/lib/ -isystem /usr/ia64-suse-linux/include -isystem /usr/ia64-suse-linux/sys-include -funwind-tables -fclasspath= -fbootclasspath=../../../libjava/classpath/lib --encoding=UTF-8 -Wno-deprecated -fbootstrap-classes -g -O2 -c -fsource-filename=/usr/local/gcc/gcc-20110719/Build/ia64-suse-linux/libjava/classpath/lib/classes -MT java/lang/ref.lo -MD -MP -MF java/lang/ref.deps @java/lang/ref.list  -fPIC -o java/lang/.libs/ref.o
> /usr/local/gcc/gcc-20110719/libjava/classpath/java/lang/ref/ReferenceQueue.java: In class 'java.lang.ref.ReferenceQueue':
> /usr/local/gcc/gcc-20110719/libjava/classpath/java/lang/ref/ReferenceQueue.java: In method 'java.lang.ref.ReferenceQueue.enqueue(java.lang.ref.Reference)':
> In file included from /usr/local/gcc/gcc-20110719/libjava/java/lang/ref/Reference.java:202:0,
>                  from /usr/local/gcc/gcc-20110719/libjava/classpath/java/lang/ref/PhantomReference.java:75,
>                  from <built-in>:6:
> /usr/local/gcc/gcc-20110719/libjava/classpath/java/lang/ref/ReferenceQueue.java:97:0: internal compiler error: in advance_target_bb, at sched-ebb.c:691
> Please submit a full bug report,
> with preprocessed source if appropriate.
> See <http://gcc.gnu.org/bugs.html> for instructions.
> make[3]: *** [java/lang/ref.lo] Error 1

I'm not even getting to that point. ia64 has been pretty broken for me
over the last couple of weeks.

checking for exception model to use... configure: error: unable to
detect exception model
make: *** [configure-target-libstdc++-v3] Error 1

Can you package this into a testcase somehow?


Bernd

Re: Correct fix for scheduler bug PR11320

[Andreas Schwab]

Bernd Schmidt <bernds@codesourcery.com> writes:

> I'm not even getting to that point. ia64 has been pretty broken for me
> over the last couple of weeks.
>
> checking for exception model to use... configure: error: unable to
> detect exception model
> make: *** [configure-target-libstdc++-v3] Error 1

Did you look at config.log?

Andreas.

-- 
Andreas Schwab, schwab@redhat.com
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."

Re: Correct fix for scheduler bug PR11320

[Bernd Schmidt]

[-- Attachment #1: Type: text/plain, Size: 2391 bytes --]

On 07/19/11 14:25, Andreas Schwab wrote:
> I'm now seeing this ICE:
> 
> /bin/sh ./libtool --tag=GCJ   --mode=compile /usr/local/gcc/gcc-20110719/Build/./gcc/gcj -B/usr/local/gcc/gcc-20110719/Build/ia64-suse-linux/libjava/ -B/usr/local/gcc/gcc-20110719/Build/./gcc/ -B/usr/ia64-suse-linux/bin/ -B/usr/ia64-suse-linux/lib/ -isystem /usr/ia64-suse-linux/include -isystem /usr/ia64-suse-linux/sys-include    -funwind-tables -fclasspath= -fbootclasspath=../../../libjava/classpath/lib --encoding=UTF-8 -Wno-deprecated -fbootstrap-classes -g -O2  -c -o java/lang/ref.lo -fsource-filename=/usr/local/gcc/gcc-20110719/Build/ia64-suse-linux/libjava/classpath/lib/classes -MT java/lang/ref.lo -MD -MP -MF java/lang/ref.deps @java/lang/ref.list
> libtool: compile:  /usr/local/gcc/gcc-20110719/Build/./gcc/gcj -B/usr/local/gcc/gcc-20110719/Build/ia64-suse-linux/libjava/ -B/usr/local/gcc/gcc-20110719/Build/./gcc/ -B/usr/ia64-suse-linux/bin/ -B/usr/ia64-suse-linux/lib/ -isystem /usr/ia64-suse-linux/include -isystem /usr/ia64-suse-linux/sys-include -funwind-tables -fclasspath= -fbootclasspath=../../../libjava/classpath/lib --encoding=UTF-8 -Wno-deprecated -fbootstrap-classes -g -O2 -c -fsource-filename=/usr/local/gcc/gcc-20110719/Build/ia64-suse-linux/libjava/classpath/lib/classes -MT java/lang/ref.lo -MD -MP -MF java/lang/ref.deps @java/lang/ref.list  -fPIC -o java/lang/.libs/ref.o
> /usr/local/gcc/gcc-20110719/libjava/classpath/java/lang/ref/ReferenceQueue.java: In class 'java.lang.ref.ReferenceQueue':
> /usr/local/gcc/gcc-20110719/libjava/classpath/java/lang/ref/ReferenceQueue.java: In method 'java.lang.ref.ReferenceQueue.enqueue(java.lang.ref.Reference)':
> In file included from /usr/local/gcc/gcc-20110719/libjava/java/lang/ref/Reference.java:202:0,
>                  from /usr/local/gcc/gcc-20110719/libjava/classpath/java/lang/ref/PhantomReference.java:75,
>                  from <built-in>:6:
> /usr/local/gcc/gcc-20110719/libjava/classpath/java/lang/ref/ReferenceQueue.java:97:0: internal compiler error: in advance_target_bb, at sched-ebb.c:691
> Please submit a full bug report,
> with preprocessed source if appropriate.
> See <http://gcc.gnu.org/bugs.html> for instructions.
> make[3]: *** [java/lang/ref.lo] Error 1

It's getting confused about loads/stores being control_flow_insns and
getting scheduled past each other nonetheless. Mind testing the following?


Bernd


[-- Attachment #2: noncall.diff --]
[-- Type: text/plain, Size: 500 bytes --]

Index: gcc/sched-ebb.c
===================================================================
--- gcc/sched-ebb.c	(revision 176579)
+++ gcc/sched-ebb.c	(working copy)
@@ -397,6 +397,9 @@ add_deps_for_risky_insns (rtx head, rtx
 	  bb = BLOCK_FOR_INSN (insn);
 	  bb->aux = last_block;
 	  last_block = bb;
+	  if (flag_non_call_exceptions && last_jump != NULL_RTX)
+	    add_dependence (insn, last_jump, REG_DEP_ANTI);
+
 	  last_jump = insn;
 	}
       else if (INSN_P (insn) && last_jump != NULL_RTX)

Re: Correct fix for scheduler bug PR11320

[Eric Botcazou]

> It's getting confused about loads/stores being control_flow_insns and
> getting scheduled past each other nonetheless. Mind testing the following?

s/flag_non_call_exceptions/cfun->can_throw_non_call_exceptions/

-- 
Eric Botcazou

Re: Correct fix for scheduler bug PR11320

[Richard Henderson]

On 07/22/2011 10:00 AM, Eric Botcazou wrote:
>> It's getting confused about loads/stores being control_flow_insns and
>> getting scheduled past each other nonetheless. Mind testing the following?
> 
> s/flag_non_call_exceptions/cfun->can_throw_non_call_exceptions/
> 

Why test either, since control_flow_insn_p has already done so?


r~

Re: Correct fix for scheduler bug PR11320

[Bernd Schmidt]

On 07/22/11 19:17, Richard Henderson wrote:
> On 07/22/2011 10:00 AM, Eric Botcazou wrote:
>>> It's getting confused about loads/stores being control_flow_insns and
>>> getting scheduled past each other nonetheless. Mind testing the following?
>>
>> s/flag_non_call_exceptions/cfun->can_throw_non_call_exceptions/
>>
> 
> Why test either, since control_flow_insn_p has already done so?

Just to save an unnecessary call to add_dependence.

Although, come to think of it, it might not be unnecessary. The reason
these two insns aren't already dependent on each other seems to be that
they have opposite conditions, and I guess that might happen with
conditional calls as well.


Bernd

Re: Correct fix for scheduler bug PR11320

[Bernd Schmidt]

[-- Attachment #1: Type: text/plain, Size: 826 bytes --]

On 07/22/11 19:23, Bernd Schmidt wrote:
> On 07/22/11 19:17, Richard Henderson wrote:
>> On 07/22/2011 10:00 AM, Eric Botcazou wrote:
>>>> It's getting confused about loads/stores being control_flow_insns and
>>>> getting scheduled past each other nonetheless. Mind testing the following?
>>>
>>> s/flag_non_call_exceptions/cfun->can_throw_non_call_exceptions/
>>>
>>
>> Why test either, since control_flow_insn_p has already done so?
> 
> Just to save an unnecessary call to add_dependence.
> 
> Although, come to think of it, it might not be unnecessary. The reason
> these two insns aren't already dependent on each other seems to be that
> they have opposite conditions, and I guess that might happen with
> conditional calls as well.

Ok, so Andreas has verified that the following fixes the problem on
ia64. Ok?


Bernd

[-- Attachment #2: noncall.diff --]
[-- Type: text/plain, Size: 648 bytes --]

	PR rtl-optimization/49900
	* sched-ebb.c (add_deps_for_risky_insns): Also add dependencies to
	ensure basic blocks stay in the same order.

Index: gcc/sched-ebb.c
===================================================================
--- gcc/sched-ebb.c	(revision 176879)
+++ gcc/sched-ebb.c	(working copy)
@@ -397,6 +397,9 @@ add_deps_for_risky_insns (rtx head, rtx
 	  bb = BLOCK_FOR_INSN (insn);
 	  bb->aux = last_block;
 	  last_block = bb;
+	  /* Ensure blocks stay in the same order.  */
+	  if (last_jump)
+	    add_dependence (insn, last_jump, REG_DEP_ANTI);
 	  last_jump = insn;
 	}
       else if (INSN_P (insn) && last_jump != NULL_RTX)

Re: Correct fix for scheduler bug PR11320

[Richard Henderson]

On 08/05/2011 10:41 AM, Bernd Schmidt wrote:
> 	PR rtl-optimization/49900
> 	* sched-ebb.c (add_deps_for_risky_insns): Also add dependencies to
> 	ensure basic blocks stay in the same order.

Ok.


r~