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~

Re: Correct fix for scheduler bug PR11320

[Bernd Schmidt]

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
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

[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

[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

[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

[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 :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  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

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

[Andreas Schwab]

Bernd Schmidt  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, sch...@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 :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  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]

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 :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  for instructions.
make[3]: *** [java/lang/ref.lo] Error 1

Andreas.

-- 
Andreas Schwab, sch...@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

[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,

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

will be relaxed to

nop
...
addly = @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

[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: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: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

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);
- 

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]

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

[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

[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

[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

[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

[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]

> 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: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

[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