Various selective scheduling fixes

Various selective scheduling fixes

[Andrey Belevantsev]

Hello,

In this thread I will be posting the patches for the fixed selective 
scheduling PRs (except the one that was already kindly checked in by Jeff). 
  The patches were tested both on x86-64 and ia64 with the following 
combination: 1) the usual bootstrap/regtest, which only utilizes sel-sched 
on its own tests, made by default to run on arm/ppc/x86-64/ia64; 2) the 
bootstrap/regtest with the second scheduler forced to sel-sched; 3) both 
schedulers forced to sel-sched.  In all cases everything seemed to be fine.

Three of the PRs are regressions, the other two showed different errors 
across the variety of releases tested by submitters;  I think all of them 
are appropriate at this stage -- they do not touch anything outside of 
selective scheduling except the first patch where a piece of code from 
sched-deps.c needs to be refactored into a function to be called from 
sel-sched.c.

Andrey

[01/05] Fix PR 64411

[Andrey Belevantsev]

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

Hello,

In this case, we get an inconsistency between the sched-deps interface, 
saying we can't move an insn writing the si register through a vector insn, 
and the liveness analysis, saying we can.  The latter doesn't take into 
account implicit_reg_pending_clobbers set calculated in sched-deps before 
register allocation.  The solution is to reflect this set in our insn data 
(sets/uses/clobbers).

Ok for trunk?

gcc/

2016-01-15  Andrey Belevantsev  <abel@ispras.ru>

	PR target/64411
	* sched-deps.c (get_implicit_reg_pending_clobbers): New function,
	factored out from ...
	(sched_analyze_insn): ... here.
	* sched-int.h (get_implicit_reg_pending_clobbers): Declare it.
	* sel-sched-ir.c (setup_id_implicit_regs): New function, use
	get_implicit_reg_pending_clobbers in it.
	(setup_id_reg_sets): Use setup_id_implicit_regs.
	(deps_init_id): Ditto.

testsuite/

2016-01-15  Andrey Belevantsev  <abel@ispras.ru>

	PR target/64411
	* gcc.target/i386/pr64411.C: New test.

Best,
Andrey

[-- Attachment #2: 01-pr64411.diff --]
[-- Type: text/x-patch, Size: 4133 bytes --]

diff --git a/gcc/sched-deps.c b/gcc/sched-deps.c
index 4961dfb..3d4a1d5 100644
--- a/gcc/sched-deps.c
+++ b/gcc/sched-deps.c
@@ -2860,6 +2860,17 @@ sched_macro_fuse_insns (rtx_insn *insn)
 
 }
 
+/* Get the implicit reg pending clobbers for INSN.  */
+void
+get_implicit_reg_pending_clobbers (rtx_insn *insn, HARD_REG_SET *temp)
+{
+  extract_insn (insn);
+  preprocess_constraints (insn);
+  alternative_mask preferred = get_preferred_alternatives (insn);
+  ira_implicitly_set_insn_hard_regs (temp, preferred);
+  AND_COMPL_HARD_REG_SET (*temp, ira_no_alloc_regs);
+}
+
 /* Analyze an INSN with pattern X to find all dependencies.  */
 static void
 sched_analyze_insn (struct deps_desc *deps, rtx x, rtx_insn *insn)
@@ -2872,12 +2883,7 @@ sched_analyze_insn (struct deps_desc *deps, rtx x, rtx_insn *insn)
   if (! reload_completed)
     {
       HARD_REG_SET temp;
-
-      extract_insn (insn);
-      preprocess_constraints (insn);
-      alternative_mask prefrred = get_preferred_alternatives (insn);
-      ira_implicitly_set_insn_hard_regs (&temp, prefrred);
-      AND_COMPL_HARD_REG_SET (temp, ira_no_alloc_regs);
+      get_implicit_reg_pending_clobbers (insn, &temp);
       IOR_HARD_REG_SET (implicit_reg_pending_clobbers, temp);
     }
 
diff --git a/gcc/sched-int.h b/gcc/sched-int.h
index 378c3aa..d0e2c0e 100644
--- a/gcc/sched-int.h
+++ b/gcc/sched-int.h
@@ -1351,6 +1351,7 @@ extern void finish_deps_global (void);
 extern void deps_analyze_insn (struct deps_desc *, rtx_insn *);
 extern void remove_from_deps (struct deps_desc *, rtx_insn *);
 extern void init_insn_reg_pressure_info (rtx_insn *);
+extern void get_implicit_reg_pending_clobbers (rtx_insn *, HARD_REG_SET *);
 
 extern dw_t get_dep_weak (ds_t, ds_t);
 extern ds_t set_dep_weak (ds_t, ds_t, dw_t);
diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
index d6c86b8..e181cb9 100644
--- a/gcc/sel-sched-ir.c
+++ b/gcc/sel-sched-ir.c
@@ -2650,6 +2650,24 @@ maybe_downgrade_id_to_use (idata_t id, insn_t insn)
     IDATA_TYPE (id) = USE;
 }
 
+/* Setup implicit register clobbers calculated by sched-deps before reload.  */
+static void
+setup_id_implicit_regs (idata_t id, insn_t insn)
+{
+  if (reload_completed)
+    return;
+
+  HARD_REG_SET temp;
+  unsigned regno;
+  hard_reg_set_iterator hrsi;
+
+  get_implicit_reg_pending_clobbers (insn, &temp);
+  EXECUTE_IF_SET_IN_HARD_REG_SET (temp, 0, regno, hrsi)
+    {
+      SET_REGNO_REG_SET (IDATA_REG_SETS (id), regno);
+    }
+}
+
 /* Setup register sets describing INSN in ID.  */
 static void
 setup_id_reg_sets (idata_t id, insn_t insn)
@@ -2704,6 +2722,9 @@ setup_id_reg_sets (idata_t id, insn_t insn)
 	}
     }
 
+  /* Also get implicit reg clobbers from sched-deps.  */
+  setup_id_implicit_regs (id, insn);
+
   return_regset_to_pool (tmp);
 }
 
@@ -2735,20 +2756,18 @@ deps_init_id (idata_t id, insn_t insn, bool force_unique_p)
   deps_init_id_data.force_use_p = false;
 
   init_deps (dc, false);
-
   memcpy (&deps_init_id_sched_deps_info,
 	  &const_deps_init_id_sched_deps_info,
 	  sizeof (deps_init_id_sched_deps_info));
-
   if (spec_info != NULL)
     deps_init_id_sched_deps_info.generate_spec_deps = 1;
-
   sched_deps_info = &deps_init_id_sched_deps_info;
 
   deps_analyze_insn (dc, insn);
+  /* Implicit reg clobbers received from sched-deps separately.  */
+  setup_id_implicit_regs (id, insn);
 
   free_deps (dc);
-
   deps_init_id_data.id = NULL;
 }
 
diff --git a/gcc/testsuite/gcc.target/i386/pr64411.C b/gcc/testsuite/gcc.target/i386/pr64411.C
new file mode 100644
index 0000000..55858fb
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr64411.C
@@ -0,0 +1,27 @@
+/* { dg-do compile } */
+/* { dg-options "-Os -mcmodel=medium -fPIC -fschedule-insns -fselective-scheduling" } */
+
+typedef __SIZE_TYPE__ size_t;
+
+extern "C"  long strtol ()
+  { return 0; }
+
+static struct {
+  void *sp[2];
+} info;
+
+union S813
+{
+  void * c[5];
+}
+s813;
+
+S813 a813[5];
+S813 check813 (S813, S813 *, S813);
+
+void checkx813 ()
+{
+  __builtin_memset (&s813, '\0', sizeof (s813));
+  __builtin_memset (&info, '\0', sizeof (info));
+  check813 (s813, &a813[1], a813[2]);
+}

Re: [01/05] Fix PR 64411

[Alexander Monakov]

On Mon, 14 Mar 2016, Andrey Belevantsev wrote:
> In this case, we get an inconsistency between the sched-deps interface, saying
> we can't move an insn writing the si register through a vector insn, and the
> liveness analysis, saying we can.  The latter doesn't take into account
> implicit_reg_pending_clobbers set calculated in sched-deps before register
> allocation.  The solution is to reflect this set in our insn data
> (sets/uses/clobbers).
> 
> Ok for trunk?

One nit; the prototype of the new function:

extern void get_implicit_reg_pending_clobbers (rtx_insn *, HARD_REG_SET *);

has source operand on the left, destination on the right; it's probably nicer
to swap them around.

OK as far as selective scheduler changes go, but this also needs a general
scheduler maintainer ack for the sched-deps.c change.  Vladimir, can you have
a look?

Thanks.
Alexander

Re: [01/05] Fix PR 64411

[Bernd Schmidt]

On 03/14/2016 05:23 PM, Alexander Monakov wrote:
> On Mon, 14 Mar 2016, Andrey Belevantsev wrote:
>> In this case, we get an inconsistency between the sched-deps interface, saying
>> we can't move an insn writing the si register through a vector insn, and the
>> liveness analysis, saying we can.  The latter doesn't take into account
>> implicit_reg_pending_clobbers set calculated in sched-deps before register
>> allocation.  The solution is to reflect this set in our insn data
>> (sets/uses/clobbers).
>>
>> Ok for trunk?
>
> One nit; the prototype of the new function:
>
> extern void get_implicit_reg_pending_clobbers (rtx_insn *, HARD_REG_SET *);
>
> has source operand on the left, destination on the right; it's probably nicer
> to swap them around.
>
> OK as far as selective scheduler changes go, but this also needs a general
> scheduler maintainer ack for the sched-deps.c change.  Vladimir, can you have
> a look?

Needs better documentation of the new function's arguments (as per 
general requirements for such things), but otherwise that part is ok 
(either arg order). The sel-sched parts should also have proper function 
comments however, and here:

+    {
+      SET_REGNO_REG_SET (IDATA_REG_SETS (id), regno);
+    }

we don't use braces around single statements.


Bernd

Re: [01/05] Fix PR 64411

[Andrey Belevantsev]

Hello,

On 14.03.2016 19:45, Bernd Schmidt wrote:
> On 03/14/2016 05:23 PM, Alexander Monakov wrote:
>> On Mon, 14 Mar 2016, Andrey Belevantsev wrote:
>>> In this case, we get an inconsistency between the sched-deps interface,
>>> saying
>>> we can't move an insn writing the si register through a vector insn, and
>>> the
>>> liveness analysis, saying we can.  The latter doesn't take into account
>>> implicit_reg_pending_clobbers set calculated in sched-deps before register
>>> allocation.  The solution is to reflect this set in our insn data
>>> (sets/uses/clobbers).
>>>
>>> Ok for trunk?
>>
>> One nit; the prototype of the new function:
>>
>> extern void get_implicit_reg_pending_clobbers (rtx_insn *, HARD_REG_SET *);
>>
>> has source operand on the left, destination on the right; it's probably
>> nicer
>> to swap them around.
>>
>> OK as far as selective scheduler changes go, but this also needs a general
>> scheduler maintainer ack for the sched-deps.c change.  Vladimir, can you
>> have
>> a look?
>
> Needs better documentation of the new function's arguments (as per general
> requirements for such things), but otherwise that part is ok (either arg
> order). The sel-sched parts should also have proper function comments
> however, and here:
>
> +    {
> +      SET_REGNO_REG_SET (IDATA_REG_SETS (id), regno);
> +    }
>
> we don't use braces around single statements.

I've incorporated both yours and Alexander's comments and committed the 
patch as rev. 234216.

Andrey

>
>
> Bernd

[02/05] Fix PR 63384

[Andrey Belevantsev]

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

Hello,

Here we're looping because we decrease the counter of the insns we still 
can issue on a DEBUG_INSN thus rendering the counter negative.  The fix is 
to not count debug insns in the corresponding code.  The selective 
scheduling is known to spoil the result of var tracking, but still it is 
not the reason to hang in there.

The toggle option used in the test seems to be the equivalent of just 
enabling var-tracking-assignments which should lead to the same situation; 
however, if specified as is, var-tracking-assignments will be disabled by 
the toplev.c:1460 code.  Maybe we also need the same treatment for 
flag_var_tracking_assignments_toggle.

Ok for trunk?

gcc/

2016-03-14  Andrey Belevantsev  <abel@ispras.ru>

     PR rtl-optimization/63384
     * sel-sched.c (invoke_aftermath_hooks): Do not decrease issue_more on 
DEBUG_INSN_P insns.

testsuite/

2016-03-14  Andrey Belevantsev  <abel@ispras.ru>

     PR rtl-optimization/63384
     * testsuite/g++.dg/pr63384.C: New test.

Best,
Andrey


[-- Attachment #2: 02-pr63384.diff --]
[-- Type: text/x-patch, Size: 1232 bytes --]

diff --git a/gcc/sel-sched.c b/gcc/sel-sched.c
index c798935..893a3e5 100644
--- a/gcc/sel-sched.c
+++ b/gcc/sel-sched.c
@@ -4249,7 +4249,8 @@ invoke_aftermath_hooks (fence_t fence, rtx_insn *best_insn, int issue_more)
                                       issue_more);
       memcpy (FENCE_STATE (fence), curr_state, dfa_state_size);
     }
-  else if (GET_CODE (PATTERN (best_insn)) != USE
+  else if (! DEBUG_INSN_P (best_insn)
+	   && GET_CODE (PATTERN (best_insn)) != USE
            && GET_CODE (PATTERN (best_insn)) != CLOBBER)
     issue_more--;
 
diff --git a/gcc/testsuite/g++.dg/pr63384.C b/gcc/testsuite/g++.dg/pr63384.C
new file mode 100644
index 0000000..b4e0784
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr63384.C
@@ -0,0 +1,12 @@
+/* { dg-do compile { target powerpc*-*-* ia64-*-* i?86-*-* x86_64-*-* } } */
+/* { dg-options "-O2 -fselective-scheduling2 -fsel-sched-pipelining  -fsel-sched-pipelining-outer-loops -fsel-sched-reschedule-pipelined -fvar-tracking-assignments-toggle -ftree-vectorize" } */
+
+template <class T> T **make_test_matrix() {
+ T **data = new T *;
+ for (int i = 0; i < 1000; i++)
+    ;
+}
+
+template <typename T> void test() { T **c = make_test_matrix<T>(); }
+
+main() { test<float>(); }

Re: [02/05] Fix PR 63384

[Alexander Monakov]

On Mon, 14 Mar 2016, Andrey Belevantsev wrote:
> Here we're looping because we decrease the counter of the insns we still can
> issue on a DEBUG_INSN thus rendering the counter negative.  The fix is to not
> count debug insns in the corresponding code.  The selective scheduling is
> known to spoil the result of var tracking, but still it is not the reason to
> hang in there.
> 
> The toggle option used in the test seems to be the equivalent of just enabling
> var-tracking-assignments which should lead to the same situation; however, if
> specified as is, var-tracking-assignments will be disabled by the
> toplev.c:1460 code.  Maybe we also need the same treatment for
> flag_var_tracking_assignments_toggle.

Hm, I've tried running the test by hand, and I don't follow you: it loops with
either -fvta or -fvta-toggle, producing the expected warning; and doesn't loop
with just -fvar-tracking, when VTA is implicitely disabled at toplev.c:1460.
Sorry, I might have misled you about this (off-list), but it seems toplev.c is
actually working as intended here.

> Ok for trunk?

OK with the formatting oddity fixed:

diff --git a/gcc/sel-sched.c b/gcc/sel-sched.c
index c798935..893a3e5 100644
--- a/gcc/sel-sched.c
+++ b/gcc/sel-sched.c
@@ -4249,7 +4249,8 @@ invoke_aftermath_hooks (fence_t fence, rtx_insn *best_insn, int issue_more)
                                       issue_more);
       memcpy (FENCE_STATE (fence), curr_state, dfa_state_size);
     }
-  else if (GET_CODE (PATTERN (best_insn)) != USE
+  else if (! DEBUG_INSN_P (best_insn)
+	   && GET_CODE (PATTERN (best_insn)) != USE
            && GET_CODE (PATTERN (best_insn)) != CLOBBER)
     issue_more--;
 
The prevailing style is '!DEBUG_INSN_P' (no space); it's probably better to
use the same indent style (spaces) on the two following lines too.

Thanks.
Alexander

Re: [02/05] Fix PR 63384

[Marek Polacek]

On Mon, Mar 14, 2016 at 12:31:24PM +0300, Andrey Belevantsev wrote:
> Hello,
> 
> Here we're looping because we decrease the counter of the insns we still can
> issue on a DEBUG_INSN thus rendering the counter negative.  The fix is to
> not count debug insns in the corresponding code.  The selective scheduling
> is known to spoil the result of var tracking, but still it is not the reason
> to hang in there.
> 
> The toggle option used in the test seems to be the equivalent of just
> enabling var-tracking-assignments which should lead to the same situation;
> however, if specified as is, var-tracking-assignments will be disabled by
> the toplev.c:1460 code.  Maybe we also need the same treatment for
> flag_var_tracking_assignments_toggle.
> 
> Ok for trunk?
> 
> gcc/
> 
> 2016-03-14  Andrey Belevantsev  <abel@ispras.ru>
> 
>     PR rtl-optimization/63384
>     * sel-sched.c (invoke_aftermath_hooks): Do not decrease issue_more on
> DEBUG_INSN_P insns.
> 
> testsuite/
> 
> 2016-03-14  Andrey Belevantsev  <abel@ispras.ru>
> 
>     PR rtl-optimization/63384
>     * testsuite/g++.dg/pr63384.C: New test.
> 
> Best,
> Andrey
> 

> diff --git a/gcc/sel-sched.c b/gcc/sel-sched.c
> index c798935..893a3e5 100644
> --- a/gcc/sel-sched.c
> +++ b/gcc/sel-sched.c
> @@ -4249,7 +4249,8 @@ invoke_aftermath_hooks (fence_t fence, rtx_insn *best_insn, int issue_more)
>                                        issue_more);
>        memcpy (FENCE_STATE (fence), curr_state, dfa_state_size);
>      }
> -  else if (GET_CODE (PATTERN (best_insn)) != USE
> +  else if (! DEBUG_INSN_P (best_insn)
> +	   && GET_CODE (PATTERN (best_insn)) != USE
>             && GET_CODE (PATTERN (best_insn)) != CLOBBER)
>      issue_more--;
>  
> diff --git a/gcc/testsuite/g++.dg/pr63384.C b/gcc/testsuite/g++.dg/pr63384.C
> new file mode 100644
> index 0000000..b4e0784
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/pr63384.C
> @@ -0,0 +1,12 @@
> +/* { dg-do compile { target powerpc*-*-* ia64-*-* i?86-*-* x86_64-*-* } } */
> +/* { dg-options "-O2 -fselective-scheduling2 -fsel-sched-pipelining  -fsel-sched-pipelining-outer-loops -fsel-sched-reschedule-pipelined -fvar-tracking-assignments-toggle -ftree-vectorize" } */
> +
> +template <class T> T **make_test_matrix() {
> + T **data = new T *;
> + for (int i = 0; i < 1000; i++)
> +    ;
> +}
> +
> +template <typename T> void test() { T **c = make_test_matrix<T>(); }
> +
> +main() { test<float>(); }

This test fails for me due to
cc1plus: warning: var-tracking-assignments changes selective scheduling

	Marek

Re: [02/05] Fix PR 63384

[Alexander Monakov]

On Tue, 15 Mar 2016, Marek Polacek wrote:
> This test fails for me due to
> cc1plus: warning: var-tracking-assignments changes selective scheduling

Thanks for the heads-up Marek, and sorry for the trouble.  Like I said in the
adjacent reply, the warning is expected (I didn't realize the testsuite would
notice that, though).  I think the right fix is to simply add "-w" to
dg-options, and while we are at it, we should probably change -fvta-toggle to
just -fvta as well (because VTA is active either way, right?).

Andrey?

Thanks.
Alexander

Re: [02/05] Fix PR 63384

[Andrey Belevantsev]

On 15.03.2016 20:44, Alexander Monakov wrote:
> On Tue, 15 Mar 2016, Marek Polacek wrote:
>> This test fails for me due to
>> cc1plus: warning: var-tracking-assignments changes selective scheduling
>
> Thanks for the heads-up Marek, and sorry for the trouble.  Like I said in the
> adjacent reply, the warning is expected (I didn't realize the testsuite would
> notice that, though).  I think the right fix is to simply add "-w" to
> dg-options, and while we are at it, we should probably change -fvta-toggle to
> just -fvta as well (because VTA is active either way, right?).

Yes, the -fvta should work.  Sorry for the breakage, I guess I've misread 
the compare-tests output when also checking the run with forced sel-sched 
enabled.

I can take care of the test tomorrow morning or you can do it now.

Best,
Andrey

>
> Andrey?
>
> Thanks.
> Alexander
>

Re: [02/05] Fix PR 63384

[Alexander Monakov]

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

On Tue, 15 Mar 2016, Andrey Belevantsev wrote:
> On 15.03.2016 20:44, Alexander Monakov wrote:
> > On Tue, 15 Mar 2016, Marek Polacek wrote:
> > > This test fails for me due to
> > > cc1plus: warning: var-tracking-assignments changes selective scheduling
> >
> > Thanks for the heads-up Marek, and sorry for the trouble.  Like I said in
> > the adjacent reply, the warning is expected (I didn't realize the
> > testsuite would notice that, though).  I think the right fix is to simply
> > add "-w" to dg-options, and while we are at it, we should probably change
> > -fvta-toggle to just -fvta as well (because VTA is active either way,
> > right?).
> 
> Yes, the -fvta should work.  Sorry for the breakage, I guess I've misread the
> compare-tests output when also checking the run with forced sel-sched enabled.
> 
> I can take care of the test tomorrow morning or you can do it now.

Thanks for confirming — committed rev. 234227.

Alexander

[03/05] Fix PR 66660

[Andrey Belevantsev]

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

Hello,

We speculate an insn in the PR but we do not make a check for it though we 
should.  The thing that broke this was the fix for PR 45472.  In that pr, 
we have moved a volatile insn too far up because we failed to merge the 
bits describing its volatility when we have processed a control flow split. 
  The code to propagate the insn pattern with the insn merging was added 
when the volatility of the two insns from the both split branches differ. 
However, the volatility of the speculated insn and its original differ: the 
original insn may trap while the speculated version may not.  Thus, we 
replace a speculative pattern with the original one per the PR 45472 fix 
for no reason.

The patch for this problem just limits the original fix for PR 45472 to 
apply for non-speculative insns only.  There is no test as it is not so 
easy to construct one -- we could count the number of speculation check in 
the resulting assembly but there is no way to force speculation to happen.

Ok for trunk?

gcc/

2016-03-14  Andrey Belevantsev  <abel@ispras.ru>

     PR target/66660
     * sel-sched-ir.c (merge_expr): Do not propagate trap bits into 
speculative insns.

Best,
Andrey

[-- Attachment #2: 03-pr66660.diff --]
[-- Type: text/x-patch, Size: 1303 bytes --]

commit 53ef39496acc26cc0021555e403068e93343aa20
Author: Andrey Belevantsev <abel@ispras.ru>
Date:   Wed Jan 27 17:20:27 2016 +0300

    Fix pr66660: do not propagate trap bits into speculative insns

diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
index e181cb9..ec59280 100644
--- a/gcc/sel-sched-ir.c
+++ b/gcc/sel-sched-ir.c
@@ -1871,12 +1871,12 @@ merge_expr (expr_t to, expr_t from, insn_t split_point)
   /* Make sure that speculative pattern is propagated into exprs that
      have non-speculative one.  This will provide us with consistent
      speculative bits and speculative patterns inside expr.  */
-  if ((EXPR_SPEC_DONE_DS (from) != 0
-       && EXPR_SPEC_DONE_DS (to) == 0)
-      /* Do likewise for volatile insns, so that we always retain
-	 the may_trap_p bit on the resulting expression.  */
-      || (VINSN_MAY_TRAP_P (EXPR_VINSN (from))
-	  && !VINSN_MAY_TRAP_P (EXPR_VINSN (to))))
+  if (EXPR_SPEC_DONE_DS (to) == 0
+      && (EXPR_SPEC_DONE_DS (from) != 0
+	  /* Do likewise for volatile insns, so that we always retain
+	     the may_trap_p bit on the resulting expression.  */
+	  || (VINSN_MAY_TRAP_P (EXPR_VINSN (from))
+	      && !VINSN_MAY_TRAP_P (EXPR_VINSN (to)))))
     change_vinsn_in_expr (to, EXPR_VINSN (from));
 
   merge_expr_data (to, from, split_point);

Re: [03/05] Fix PR 66660

[Alexander Monakov]

On Mon, 14 Mar 2016, Andrey Belevantsev wrote:
> We speculate an insn in the PR but we do not make a check for it though we
> should.  The thing that broke this was the fix for PR 45472.  In that pr, we
> have moved a volatile insn too far up because we failed to merge the bits
> describing its volatility when we have processed a control flow split.  The
> code to propagate the insn pattern with the insn merging was added when the
> volatility of the two insns from the both split branches differ. However, the
> volatility of the speculated insn and its original differ: the original insn
> may trap while the speculated version may not.  Thus, we replace a speculative
> pattern with the original one per the PR 45472 fix for no reason.
> 
> The patch for this problem just limits the original fix for PR 45472 to apply
> for non-speculative insns only.  There is no test as it is not so easy to
> construct one -- we could count the number of speculation check in the
> resulting assembly but there is no way to force speculation to happen.
> 
> Ok for trunk?
> 
> gcc/
> 
> 2016-03-14  Andrey Belevantsev  <abel@ispras.ru>
> 
>     PR target/66660
>     * sel-sched-ir.c (merge_expr): Do not propagate trap bits into 
> speculative insns.

I think this line doesn't capture the issue at hand well; the issue is not in
propagating trap bits, but rather unintentionally dropping the speculative
pattern, right?  I'd be happier with something like "If the pattern is already
speculative, keep it, and do not check trap bits".

diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
index e181cb9..ec59280 100644
--- a/gcc/sel-sched-ir.c
+++ b/gcc/sel-sched-ir.c
@@ -1871,12 +1871,12 @@ merge_expr (expr_t to, expr_t from, insn_t split_point)
   /* Make sure that speculative pattern is propagated into exprs that
      have non-speculative one.  This will provide us with consistent
      speculative bits and speculative patterns inside expr.  */
-  if ((EXPR_SPEC_DONE_DS (from) != 0
-       && EXPR_SPEC_DONE_DS (to) == 0)
-      /* Do likewise for volatile insns, so that we always retain
-	 the may_trap_p bit on the resulting expression.  */
-      || (VINSN_MAY_TRAP_P (EXPR_VINSN (from))
-	  && !VINSN_MAY_TRAP_P (EXPR_VINSN (to))))
+  if (EXPR_SPEC_DONE_DS (to) == 0
+      && (EXPR_SPEC_DONE_DS (from) != 0
+	  /* Do likewise for volatile insns, so that we always retain
+	     the may_trap_p bit on the resulting expression.  */
+	  || (VINSN_MAY_TRAP_P (EXPR_VINSN (from))
+	      && !VINSN_MAY_TRAP_P (EXPR_VINSN (to)))))
     change_vinsn_in_expr (to, EXPR_VINSN (from));

The patch looks unusual in that it reshuffles code while keeping comments; it
seems the upper comment matches the code better now, while the lower one could
be improved to say that may_trap_p is deliberately ignored when 'to' is
already speculated.

Finally, I'd recommend to switch around the two VINSN_MAY_TRAP_P tests so that
condition on 'to' is consistently checked prior to condition on 'from'.

OK with those changes.

Thanks.
Alexander

[04/05] Fix PR 69032

[Andrey Belevantsev]

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

Hello,

We fail to find the proper seqno for the fresh bookkeeping copy in this PR. 
  The problem is that in get_seqno_by_preds we are iterating over bb from 
the given insn backwards up to the first bb insn.  We skip the initial insn 
when iterating over bb, yet we should take seqno from it.

The code in question originally didn't include bb head when iterating, and 
was patched to do so in 2011.  The patch was wrong and instead of including 
bb head managed to exclude the original insn itself.  By reading the 
original and patched code I've convinced myself that the right fix will be 
to do what the patch intended and include both the initial insn and the bb 
head in the iteration.

Ok for trunk?

gcc/

2016-03-14  Andrey Belevantsev  <abel@ispras.ru>

     PR rtl-optimization/69032
     * sel-sched-ir.c (get_seqno_by_preds): Include both tmp and head when 
looping backwards over basic block insns.

testsuite/

2016-03-14  Andrey Belevantsev  <abel@ispras.ru>

     PR rtl-optimization/69032
     * gcc.dg/pr69032.c: New test.

Best,
Andrey

[-- Attachment #2: 04-pr69032.diff --]
[-- Type: text/x-patch, Size: 977 bytes --]

diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
index ec59280..c1a9e55 100644
--- a/gcc/sel-sched-ir.c
+++ b/gcc/sel-sched-ir.c
@@ -4103,11 +4103,14 @@ get_seqno_by_preds (rtx_insn *insn)
   insn_t *preds;
   int n, i, seqno;
 
-  while (tmp != head)
+  /* Loop backwards from insn to head including both.  */
+  while (1)
     {
-      tmp = PREV_INSN (tmp);
       if (INSN_P (tmp))
         return INSN_SEQNO (tmp);
+      if (tmp == head)
+	break;
+      tmp = PREV_INSN (tmp);
     }
 
   cfg_preds (bb, &preds, &n);
diff --git a/gcc/testsuite/gcc.dg/pr69032.c b/gcc/testsuite/gcc.dg/pr69032.c
new file mode 100644
index 0000000..e0925cd
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr69032.c
@@ -0,0 +1,11 @@
+/* { dg-do compile { target powerpc*-*-* ia64-*-* i?86-*-* x86_64-*-* } } */
+/* { dg-options "-O2 -fsched-pressure -fsel-sched-pipelining -fselective-scheduling" } */
+
+void foo (long long i)
+{
+   while (i != -1)
+     {
+	++i;
+	 __asm__ ("");
+     }
+}

Re: [04/05] Fix PR 69032

[Alexander Monakov]

On Mon, 14 Mar 2016, Andrey Belevantsev wrote:
> We fail to find the proper seqno for the fresh bookkeeping copy in this PR.
> The problem is that in get_seqno_by_preds we are iterating over bb from the
> given insn backwards up to the first bb insn.  We skip the initial insn when
> iterating over bb, yet we should take seqno from it.
> 
> The code in question originally didn't include bb head when iterating, and was
> patched to do so in 2011.  The patch was wrong and instead of including bb
> head managed to exclude the original insn itself.  By reading the original and
> patched code I've convinced myself that the right fix will be to do what the
> patch intended and include both the initial insn and the bb head in the
> iteration.
> 
> Ok for trunk?
> 
> gcc/
> 
> 2016-03-14  Andrey Belevantsev  <abel@ispras.ru>
> 
>     PR rtl-optimization/69032
>     * sel-sched-ir.c (get_seqno_by_preds): Include both tmp and head when 
> looping backwards over basic block insns.

"both 'insn' and 'head'" (not tmp).

> diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
> index ec59280..c1a9e55 100644
> --- a/gcc/sel-sched-ir.c
> +++ b/gcc/sel-sched-ir.c
> @@ -4103,11 +4103,14 @@ get_seqno_by_preds (rtx_insn *insn)
>    insn_t *preds;
>    int n, i, seqno;
>  
> -  while (tmp != head)
> +  /* Loop backwards from insn to head including both.  */

"from INSN to HEAD" (uppercase).

The following structure is equivalent, but would look a bit more canonical:

  for (rtx_insn *tmp = insn; ; tmp = PREV_INSN (tmp))
    {
      if (INSN_P (tmp))
	return INSN_SEQNO (tmp);
      if (tmp == head)
	break;
    }

> +  while (1)
>      {
> -      tmp = PREV_INSN (tmp);
>        if (INSN_P (tmp))
>          return INSN_SEQNO (tmp);
> +      if (tmp == head)
> +	break;
> +      tmp = PREV_INSN (tmp);
>      }
>  
>    cfg_preds (bb, &preds, &n);

OK with formatting nits fixed ('while'/'for' spelling change at your choice).

Thanks.
Alexander

[05/05] Fix PR 69102

[Andrey Belevantsev]

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

Hello,

The problem here is readonly dependence contexts in selective scheduler. 
We're trying to cache the effect of initializing a dependence context with 
remembering that context and setting a readonly bit on it.  When first 
moving the insn 43 with REG_ARGS_SIZE note through the insn 3 (a simple eax 
set) we also set the last_args_size field of the context.  Later, when we 
make a copy of insn 43 and try to move it again through insn 3, we take the 
cached dependency context and notice the (fake) dep with last_args_size 
insn, which is the old insn 43.  Then the assert saying that we should be 
able to lift the bookkeeping copy up the same way as we did with the 
original insn breaks.

Fixed by the attached patch that makes us notice only deps with the current 
producer insn.

Ok for trunk?

gcc/

2016-03-14  Andrey Belevantsev  <abel@ispras.ru>

	PR rtl-optimization/69102
	* sel-sched.c (has_dependence_note_dep): Only take into
	account dependencies produced by the current producer insn.
	(has_dependence_note_mem_dep): Likewise.

testsuite/

2016-03-14  Andrey Belevantsev  <abel@ispras.ru>

	PR rtl-optimization/69102
	* gcc.c-torture/compile/pr69102.c: New test.

Best,
Andrey


[-- Attachment #2: 05-pr69102.diff --]
[-- Type: text/x-patch, Size: 2170 bytes --]

diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
index c1a9e55..b4aa933 100644
--- a/gcc/sel-sched-ir.c
+++ b/gcc/sel-sched-ir.c
@@ -3277,9 +3277,14 @@ has_dependence_note_reg_use (int regno)
 static void
 has_dependence_note_mem_dep (rtx mem ATTRIBUTE_UNUSED,
 			     rtx pending_mem ATTRIBUTE_UNUSED,
-			     insn_t pending_insn ATTRIBUTE_UNUSED,
+			     insn_t pending_insn,
 			     ds_t ds ATTRIBUTE_UNUSED)
 {
+  /* We're only interested in dependencies with the current producer.
+     We might get other insns that were saved in dependence context
+     as last_* or pending_* fields.  */
+  if (INSN_UID (pending_insn) != INSN_UID (has_dependence_data.pro))
+    return;
   if (!sched_insns_conditions_mutex_p (has_dependence_data.pro,
 				       VINSN_INSN_RTX (has_dependence_data.con)))
     {
@@ -3291,9 +3296,14 @@ has_dependence_note_mem_dep (rtx mem ATTRIBUTE_UNUSED,
 
 /* Note a dependence.  */
 static void
-has_dependence_note_dep (insn_t pro ATTRIBUTE_UNUSED,
+has_dependence_note_dep (insn_t pro,
 			 ds_t ds ATTRIBUTE_UNUSED)
 {
+  /* We're only interested in dependencies with the current producer.
+     We might get other insns that were saved in dependence context
+     as last_* or pending_* fields.  */
+  if (INSN_UID (pro) != INSN_UID (has_dependence_data.pro))
+    return;
   if (!sched_insns_conditions_mutex_p (has_dependence_data.pro,
 				       VINSN_INSN_RTX (has_dependence_data.con)))
     {
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr69102.c b/gcc/testsuite/gcc.c-torture/compile/pr69102.c
new file mode 100644
index 0000000..b1328ca
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr69102.c
@@ -0,0 +1,21 @@
+/* { dg-options "-Og -fPIC -fschedule-insns2 -fselective-scheduling2 -fno-tree-fre --param=max-sched-extend-regions-iters=10" } */
+void bar (unsigned int);
+
+void
+foo (void)
+{
+  char buf[1] = { 3 };
+  const char *p = buf;
+  const char **q = &p;
+  unsigned int ch;
+  switch (**q)
+    {
+    case 1:  ch = 5; break;
+    case 2:  ch = 4; break;
+    case 3:  ch = 3; break;
+    case 4:  ch = 2; break;
+    case 5:  ch = 1; break;
+    default: ch = 0; break;
+    }
+  bar (ch);
+}

Re: [05/05] Fix PR 69102

[Andrey Belevantsev]

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

Hello,

On 14.03.2016 12:52, Andrey Belevantsev wrote:
> Hello,
>
> The problem here is readonly dependence contexts in selective scheduler.
> We're trying to cache the effect of initializing a dependence context with
> remembering that context and setting a readonly bit on it.  When first
> moving the insn 43 with REG_ARGS_SIZE note through the insn 3 (a simple eax
> set) we also set the last_args_size field of the context.  Later, when we
> make a copy of insn 43 and try to move it again through insn 3, we take the
> cached dependency context and notice the (fake) dep with last_args_size
> insn, which is the old insn 43.  Then the assert saying that we should be
> able to lift the bookkeeping copy up the same way as we did with the
> original insn breaks.
>
> Fixed by the attached patch that makes us notice only deps with the current
> producer insn.
>
> Ok for trunk?

We've discussed the patch with Alexander a bit and decided to take the 
different approach.  The core issue here is not handling the new 
last_args_size field in the readonly contexts.  In general, the readonly 
context approach, when analyzing an insn with a readonly context, would 
create the necessary dependencies with all of the last_ fields but refrain 
from modifying those fields.  The reason is we need to capture the effect 
of only the single producer in the readonly context.  Failing to do so may 
update the last_ fields with the effect of subsequent analyses and having 
the fake dependencies with the insns that got into those fields instead of 
having the dependencies with the currently used producer.

So the right fix here is to guard setting of the last_args_size field with 
!deps->readonly test as it is done elsewhere in the sched-deps.c.  In stage 
1 we will also want to set the asserts in the sel-sched dependency hooks 
(where I have placed early returns in the previous version of the patch) 
actually checking that the dependency is always created with the current 
producer, and such cases will be caught sooner.

The new patch bootstrapped and tested on x86-64 with selective scheduler 
forced enabled with no regressions.  It needs the maintainer outside of 
sel-sched as we touch sched-deps.c file.  Ok for trunk?  The test is the 
same as in previous patch.

Andrey

2016-03-15  Andrey Belevantsev  <abel@ispras.ru>

         PR rtl-optimization/69102
         * sched-deps.c (sched_analyze_insn): Do not set last_args_size field
         when we have a readonly dependency context.

>
> gcc/
>
> 2016-03-14  Andrey Belevantsev  <abel@ispras.ru>
>
>     PR rtl-optimization/69102
>     * sel-sched.c (has_dependence_note_dep): Only take into
>     account dependencies produced by the current producer insn.
>     (has_dependence_note_mem_dep): Likewise.
>
> testsuite/
>
> 2016-03-14  Andrey Belevantsev  <abel@ispras.ru>
>
>     PR rtl-optimization/69102
>     * gcc.c-torture/compile/pr69102.c: New test.
>
> Best,
> Andrey
>


[-- Attachment #2: 05-pr69102-2.diff --]
[-- Type: text/x-patch, Size: 424 bytes --]

diff --git a/gcc/sched-deps.c b/gcc/sched-deps.c
index 3d4a1d5..77ffcd0 100644
--- a/gcc/sched-deps.c
+++ b/gcc/sched-deps.c
@@ -3495,7 +3495,8 @@ sched_analyze_insn (struct deps_desc *deps, rtx x, rtx_insn *insn)
     {
       if (deps->last_args_size)
 	add_dependence (insn, deps->last_args_size, REG_DEP_OUTPUT);
-      deps->last_args_size = insn;
+      if (!deps->readonly)
+	deps->last_args_size = insn;
     }
 }
 

Re: [05/05] Fix PR 69102

[Jeff Law]

On 03/15/2016 09:55 AM, Andrey Belevantsev wrote:
> Hello,
>
> On 14.03.2016 12:52, Andrey Belevantsev wrote:
>> Hello,
>>
>> The problem here is readonly dependence contexts in selective scheduler.
>> We're trying to cache the effect of initializing a dependence context
>> with
>> remembering that context and setting a readonly bit on it.  When first
>> moving the insn 43 with REG_ARGS_SIZE note through the insn 3 (a
>> simple eax
>> set) we also set the last_args_size field of the context.  Later, when we
>> make a copy of insn 43 and try to move it again through insn 3, we
>> take the
>> cached dependency context and notice the (fake) dep with last_args_size
>> insn, which is the old insn 43.  Then the assert saying that we should be
>> able to lift the bookkeeping copy up the same way as we did with the
>> original insn breaks.
>>
>> Fixed by the attached patch that makes us notice only deps with the
>> current
>> producer insn.
>>
>> Ok for trunk?
>
> We've discussed the patch with Alexander a bit and decided to take the
> different approach.  The core issue here is not handling the new
> last_args_size field in the readonly contexts.  In general, the readonly
> context approach, when analyzing an insn with a readonly context, would
> create the necessary dependencies with all of the last_ fields but
> refrain from modifying those fields.  The reason is we need to capture
> the effect of only the single producer in the readonly context.  Failing
> to do so may update the last_ fields with the effect of subsequent
> analyses and having the fake dependencies with the insns that got into
> those fields instead of having the dependencies with the currently used
> producer.
>
> So the right fix here is to guard setting of the last_args_size field
> with !deps->readonly test as it is done elsewhere in the sched-deps.c.
> In stage 1 we will also want to set the asserts in the sel-sched
> dependency hooks (where I have placed early returns in the previous
> version of the patch) actually checking that the dependency is always
> created with the current producer, and such cases will be caught sooner.
>
> The new patch bootstrapped and tested on x86-64 with selective scheduler
> forced enabled with no regressions.  It needs the maintainer outside of
> sel-sched as we touch sched-deps.c file.  Ok for trunk?  The test is the
> same as in previous patch.
>
> Andrey
>
> 2016-03-15  Andrey Belevantsev  <abel@ispras.ru>
>
>          PR rtl-optimization/69102
>          * sched-deps.c (sched_analyze_insn): Do not set last_args_size
> field
>          when we have a readonly dependency context.
>
>>
>> gcc/
>>
>> 2016-03-14  Andrey Belevantsev  <abel@ispras.ru>
>>
>>     PR rtl-optimization/69102
>>     * sel-sched.c (has_dependence_note_dep): Only take into
>>     account dependencies produced by the current producer insn.
>>     (has_dependence_note_mem_dep): Likewise.
>>
>> testsuite/
>>
>> 2016-03-14  Andrey Belevantsev  <abel@ispras.ru>
>>
>>     PR rtl-optimization/69102
>>     * gcc.c-torture/compile/pr69102.c: New test.
OK.
jeff

Re: Various selective scheduling fixes

[Andrey Belevantsev]

Hello,

On 14.03.2016 12:10, Andrey Belevantsev wrote:
> Hello,
>
> In this thread I will be posting the patches for the fixed selective
> scheduling PRs (except the one that was already kindly checked in by Jeff).
>  The patches were tested both on x86-64 and ia64 with the following
> combination: 1) the usual bootstrap/regtest, which only utilizes sel-sched
> on its own tests, made by default to run on arm/ppc/x86-64/ia64; 2) the
> bootstrap/regtest with the second scheduler forced to sel-sched; 3) both
> schedulers forced to sel-sched.  In all cases everything seemed to be fine.
>
> Three of the PRs are regressions, the other two showed different errors
> across the variety of releases tested by submitters;  I think all of them
> are appropriate at this stage -- they do not touch anything outside of
> selective scheduling except the first patch where a piece of code from
> sched-deps.c needs to be refactored into a function to be called from
> sel-sched.c.

I've backported all regression PRs to gcc-5-branch after testing there 
again with selective scheduling force enabled: PRs 64411, 66660, 69032, 
69102.  The first one was not marked as a regression as such but the test 
for PR 70292, which is duplicate, works for me on gcc 5.1 thus making it a 
regression, too.

Andrey

>
> Andrey

Re: Various selective scheduling fixes

[Christophe Lyon]

On 31 March 2016 at 16:43, Andrey Belevantsev <abel@ispras.ru> wrote:
> Hello,
>
> On 14.03.2016 12:10, Andrey Belevantsev wrote:
>>
>> Hello,
>>
>> In this thread I will be posting the patches for the fixed selective
>> scheduling PRs (except the one that was already kindly checked in by
>> Jeff).
>>  The patches were tested both on x86-64 and ia64 with the following
>> combination: 1) the usual bootstrap/regtest, which only utilizes sel-sched
>> on its own tests, made by default to run on arm/ppc/x86-64/ia64; 2) the
>> bootstrap/regtest with the second scheduler forced to sel-sched; 3) both
>> schedulers forced to sel-sched.  In all cases everything seemed to be
>> fine.
>>
>> Three of the PRs are regressions, the other two showed different errors
>> across the variety of releases tested by submitters;  I think all of them
>> are appropriate at this stage -- they do not touch anything outside of
>> selective scheduling except the first patch where a piece of code from
>> sched-deps.c needs to be refactored into a function to be called from
>> sel-sched.c.
>
>
> I've backported all regression PRs to gcc-5-branch after testing there again
> with selective scheduling force enabled: PRs 64411, 66660, 69032, 69102.
> The first one was not marked as a regression as such but the test for PR
> 70292, which is duplicate, works for me on gcc 5.1 thus making it a
> regression, too.
>

Hi,

The backport for pr69102 shows that the new testcase fails to compile (ICE)
when GCC is configured as:

--target=arm-none-linux-gnueabihf --with-float=hard --with-mode=arm
--with-cpu=cortex-a15 --with-fpu=neon-vfpv4

/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/compile/pr69102.c:
In function 'foo':
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/compile/pr69102.c:21:1:
internal compiler error: Segmentation fault
0xa64d15 crash_signal
        /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/toplev.c:383
0xfa41d7 autopref_multipass_dfa_lookahead_guard(rtx_insn*, int)
        /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/haifa-sched.c:5752
0xa31cd2 invoke_dfa_lookahead_guard
        /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:4212
0xa31cd2 find_best_expr
        /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:4415
0xa343fb fill_insns
        /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:5570
0xa343fb schedule_on_fences
        /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7395
0xa36010 sel_sched_region_2
        /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7533
0xa36f2a sel_sched_region_1
        /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7575
0xa36f2a sel_sched_region(int)
        /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7676
0xa37589 run_selective_scheduling()
        /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7752
0xa14aed rest_of_handle_sched2
        /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sched-rgn.c:3647
0xa14aed execute
        /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sched-rgn.c:3791

See http://people.linaro.org/~christophe.lyon/cross-validation/gcc/gcc-5-branch/234625/arm-none-linux-gnueabihf/diff-gcc-rh60-arm-none-linux-gnueabihf-arm-cortex-a15-neon-vfpv4.txt

Can you have a look?

Christophe

> Andrey
>
>>
>> Andrey
>
>

Re: Various selective scheduling fixes

[Andrey Belevantsev]

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

Hi Christophe,

On 01.04.2016 10:33, Christophe Lyon wrote:
> On 31 March 2016 at 16:43, Andrey Belevantsev <abel@ispras.ru> wrote:
>> Hello,
>>
>> On 14.03.2016 12:10, Andrey Belevantsev wrote:
>>>
>>> Hello,
>>>
>>> In this thread I will be posting the patches for the fixed selective
>>> scheduling PRs (except the one that was already kindly checked in by
>>> Jeff).
>>>  The patches were tested both on x86-64 and ia64 with the following
>>> combination: 1) the usual bootstrap/regtest, which only utilizes sel-sched
>>> on its own tests, made by default to run on arm/ppc/x86-64/ia64; 2) the
>>> bootstrap/regtest with the second scheduler forced to sel-sched; 3) both
>>> schedulers forced to sel-sched.  In all cases everything seemed to be
>>> fine.
>>>
>>> Three of the PRs are regressions, the other two showed different errors
>>> across the variety of releases tested by submitters;  I think all of them
>>> are appropriate at this stage -- they do not touch anything outside of
>>> selective scheduling except the first patch where a piece of code from
>>> sched-deps.c needs to be refactored into a function to be called from
>>> sel-sched.c.
>>
>>
>> I've backported all regression PRs to gcc-5-branch after testing there again
>> with selective scheduling force enabled: PRs 64411, 66660, 69032, 69102.
>> The first one was not marked as a regression as such but the test for PR
>> 70292, which is duplicate, works for me on gcc 5.1 thus making it a
>> regression, too.
>>
>
> Hi,
>
> The backport for pr69102 shows that the new testcase fails to compile (ICE)
> when GCC is configured as:
>
> --target=arm-none-linux-gnueabihf --with-float=hard --with-mode=arm
> --with-cpu=cortex-a15 --with-fpu=neon-vfpv4
>
> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/compile/pr69102.c:
> In function 'foo':
> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/compile/pr69102.c:21:1:
> internal compiler error: Segmentation fault
> 0xa64d15 crash_signal
>         /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/toplev.c:383
> 0xfa41d7 autopref_multipass_dfa_lookahead_guard(rtx_insn*, int)
>         /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/haifa-sched.c:5752
> 0xa31cd2 invoke_dfa_lookahead_guard
>         /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:4212
> 0xa31cd2 find_best_expr
>         /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:4415
> 0xa343fb fill_insns
>         /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:5570
> 0xa343fb schedule_on_fences
>         /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7395
> 0xa36010 sel_sched_region_2
>         /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7533
> 0xa36f2a sel_sched_region_1
>         /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7575
> 0xa36f2a sel_sched_region(int)
>         /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7676
> 0xa37589 run_selective_scheduling()
>         /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7752
> 0xa14aed rest_of_handle_sched2
>         /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sched-rgn.c:3647
> 0xa14aed execute
>         /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sched-rgn.c:3791
>
> See http://people.linaro.org/~christophe.lyon/cross-validation/gcc/gcc-5-branch/234625/arm-none-linux-gnueabihf/diff-gcc-rh60-arm-none-linux-gnueabihf-arm-cortex-a15-neon-vfpv4.txt
>
> Can you have a look?

That's because A15 is the only place which enables 
autopref_multipass_dfa_lookahead_guard as the DFA lookahead guard hook. 
But autoprefetch modeling doesn't work for selective scheduling, it uses 
haifa structures that are not kept up to date during sel-sched.  So this is 
not supposed to work as soon as the param value for prefetcher lookahead 
depth is positive.

The following patch works for me.  Could you check it with your testing? 
If it works fine for you, I would install the patch both for trunk and 
gcc-5.  It would be great to force sel-sched to be enabled, too.  I could 
do that but I don't have the hardware or cross-arm target tools at the moment.

         * haifa-sched.c (autopref_multipass_dfa_lookahead_guard): Disable 
for selective scheduler.

Best,
Andrey


>
> Christophe
>
>> Andrey
>>
>>>
>>> Andrey
>>
>>


[-- Attachment #2: disable-autopref-sel-sched.diff --]
[-- Type: text/x-patch, Size: 437 bytes --]

diff --git a/gcc/haifa-sched.c b/gcc/haifa-sched.c
index ad2450b..c790830 100644
--- a/gcc/haifa-sched.c
+++ b/gcc/haifa-sched.c
@@ -5691,6 +5691,10 @@ autopref_multipass_dfa_lookahead_guard (rtx_insn *insn1, int ready_index)
 {
   int r = 0;
 
+  /* Autoprefetcher modeling is not supported by selective scheduler.  */
+  if (sel_sched_p ())
+    return 0;
+
   if (PARAM_VALUE (PARAM_SCHED_AUTOPREF_QUEUE_DEPTH) <= 0)
     return 0;
 

Re: Various selective scheduling fixes

[Christophe Lyon]

On 1 April 2016 at 10:54, Andrey Belevantsev <abel@ispras.ru> wrote:
> Hi Christophe,
>
>
> On 01.04.2016 10:33, Christophe Lyon wrote:
>>
>> On 31 March 2016 at 16:43, Andrey Belevantsev <abel@ispras.ru> wrote:
>>>
>>> Hello,
>>>
>>> On 14.03.2016 12:10, Andrey Belevantsev wrote:
>>>>
>>>>
>>>> Hello,
>>>>
>>>> In this thread I will be posting the patches for the fixed selective
>>>> scheduling PRs (except the one that was already kindly checked in by
>>>> Jeff).
>>>>  The patches were tested both on x86-64 and ia64 with the following
>>>> combination: 1) the usual bootstrap/regtest, which only utilizes
>>>> sel-sched
>>>> on its own tests, made by default to run on arm/ppc/x86-64/ia64; 2) the
>>>> bootstrap/regtest with the second scheduler forced to sel-sched; 3) both
>>>> schedulers forced to sel-sched.  In all cases everything seemed to be
>>>> fine.
>>>>
>>>> Three of the PRs are regressions, the other two showed different errors
>>>> across the variety of releases tested by submitters;  I think all of
>>>> them
>>>> are appropriate at this stage -- they do not touch anything outside of
>>>> selective scheduling except the first patch where a piece of code from
>>>> sched-deps.c needs to be refactored into a function to be called from
>>>> sel-sched.c.
>>>
>>>
>>>
>>> I've backported all regression PRs to gcc-5-branch after testing there
>>> again
>>> with selective scheduling force enabled: PRs 64411, 66660, 69032, 69102.
>>> The first one was not marked as a regression as such but the test for PR
>>> 70292, which is duplicate, works for me on gcc 5.1 thus making it a
>>> regression, too.
>>>
>>
>> Hi,
>>
>> The backport for pr69102 shows that the new testcase fails to compile
>> (ICE)
>> when GCC is configured as:
>>
>> --target=arm-none-linux-gnueabihf --with-float=hard --with-mode=arm
>> --with-cpu=cortex-a15 --with-fpu=neon-vfpv4
>>
>>
>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/compile/pr69102.c:
>> In function 'foo':
>>
>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/compile/pr69102.c:21:1:
>> internal compiler error: Segmentation fault
>> 0xa64d15 crash_signal
>>         /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/toplev.c:383
>> 0xfa41d7 autopref_multipass_dfa_lookahead_guard(rtx_insn*, int)
>>         /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/haifa-sched.c:5752
>> 0xa31cd2 invoke_dfa_lookahead_guard
>>         /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:4212
>> 0xa31cd2 find_best_expr
>>         /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:4415
>> 0xa343fb fill_insns
>>         /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:5570
>> 0xa343fb schedule_on_fences
>>         /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7395
>> 0xa36010 sel_sched_region_2
>>         /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7533
>> 0xa36f2a sel_sched_region_1
>>         /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7575
>> 0xa36f2a sel_sched_region(int)
>>         /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7676
>> 0xa37589 run_selective_scheduling()
>>         /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7752
>> 0xa14aed rest_of_handle_sched2
>>         /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sched-rgn.c:3647
>> 0xa14aed execute
>>         /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sched-rgn.c:3791
>>
>> See
>> http://people.linaro.org/~christophe.lyon/cross-validation/gcc/gcc-5-branch/234625/arm-none-linux-gnueabihf/diff-gcc-rh60-arm-none-linux-gnueabihf-arm-cortex-a15-neon-vfpv4.txt
>>
>> Can you have a look?
>
>
> That's because A15 is the only place which enables
> autopref_multipass_dfa_lookahead_guard as the DFA lookahead guard hook. But
> autoprefetch modeling doesn't work for selective scheduling, it uses haifa
> structures that are not kept up to date during sel-sched.  So this is not
> supposed to work as soon as the param value for prefetcher lookahead depth
> is positive.
>
> The following patch works for me.  Could you check it with your testing? If
> it works fine for you, I would install the patch both for trunk and gcc-5.
> It would be great to force sel-sched to be enabled, too.  I could do that
> but I don't have the hardware or cross-arm target tools at the moment.
>
>         * haifa-sched.c (autopref_multipass_dfa_lookahead_guard): Disable
> for selective scheduler.
>

It does work for me, it also fixes the other ICE I reported (on pr69307).

But note that both tests pass on trunk.

Christophe


> Best,
> Andrey
>
>
>>
>> Christophe
>>
>>> Andrey
>>>
>>>>
>>>> Andrey
>>>
>>>
>>>
>

Re: Various selective scheduling fixes

[Kyrill Tkachov]

Hi Christophe, Andrey,

On 01/04/16 14:09, Christophe Lyon wrote:
> On 1 April 2016 at 10:54, Andrey Belevantsev <abel@ispras.ru> wrote:
>> Hi Christophe,
>>
>>
>> On 01.04.2016 10:33, Christophe Lyon wrote:
>>> On 31 March 2016 at 16:43, Andrey Belevantsev <abel@ispras.ru> wrote:
>>>> Hello,
>>>>
>>>> On 14.03.2016 12:10, Andrey Belevantsev wrote:
>>>>>
>>>>> Hello,
>>>>>
>>>>> In this thread I will be posting the patches for the fixed selective
>>>>> scheduling PRs (except the one that was already kindly checked in by
>>>>> Jeff).
>>>>>   The patches were tested both on x86-64 and ia64 with the following
>>>>> combination: 1) the usual bootstrap/regtest, which only utilizes
>>>>> sel-sched
>>>>> on its own tests, made by default to run on arm/ppc/x86-64/ia64; 2) the
>>>>> bootstrap/regtest with the second scheduler forced to sel-sched; 3) both
>>>>> schedulers forced to sel-sched.  In all cases everything seemed to be
>>>>> fine.
>>>>>
>>>>> Three of the PRs are regressions, the other two showed different errors
>>>>> across the variety of releases tested by submitters;  I think all of
>>>>> them
>>>>> are appropriate at this stage -- they do not touch anything outside of
>>>>> selective scheduling except the first patch where a piece of code from
>>>>> sched-deps.c needs to be refactored into a function to be called from
>>>>> sel-sched.c.
>>>>
>>>>
>>>> I've backported all regression PRs to gcc-5-branch after testing there
>>>> again
>>>> with selective scheduling force enabled: PRs 64411, 66660, 69032, 69102.
>>>> The first one was not marked as a regression as such but the test for PR
>>>> 70292, which is duplicate, works for me on gcc 5.1 thus making it a
>>>> regression, too.
>>>>
>>> Hi,
>>>
>>> The backport for pr69102 shows that the new testcase fails to compile
>>> (ICE)
>>> when GCC is configured as:
>>>
>>> --target=arm-none-linux-gnueabihf --with-float=hard --with-mode=arm
>>> --with-cpu=cortex-a15 --with-fpu=neon-vfpv4
>>>
>>>
>>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/compile/pr69102.c:
>>> In function 'foo':
>>>
>>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/compile/pr69102.c:21:1:
>>> internal compiler error: Segmentation fault
>>> 0xa64d15 crash_signal
>>>          /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/toplev.c:383
>>> 0xfa41d7 autopref_multipass_dfa_lookahead_guard(rtx_insn*, int)
>>>          /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/haifa-sched.c:5752
>>> 0xa31cd2 invoke_dfa_lookahead_guard
>>>          /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:4212
>>> 0xa31cd2 find_best_expr
>>>          /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:4415
>>> 0xa343fb fill_insns
>>>          /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:5570
>>> 0xa343fb schedule_on_fences
>>>          /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7395
>>> 0xa36010 sel_sched_region_2
>>>          /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7533
>>> 0xa36f2a sel_sched_region_1
>>>          /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7575
>>> 0xa36f2a sel_sched_region(int)
>>>          /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7676
>>> 0xa37589 run_selective_scheduling()
>>>          /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7752
>>> 0xa14aed rest_of_handle_sched2
>>>          /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sched-rgn.c:3647
>>> 0xa14aed execute
>>>          /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sched-rgn.c:3791
>>>
>>> See
>>> http://people.linaro.org/~christophe.lyon/cross-validation/gcc/gcc-5-branch/234625/arm-none-linux-gnueabihf/diff-gcc-rh60-arm-none-linux-gnueabihf-arm-cortex-a15-neon-vfpv4.txt
>>>
>>> Can you have a look?
>>
>> That's because A15 is the only place which enables
>> autopref_multipass_dfa_lookahead_guard as the DFA lookahead guard hook. But
>> autoprefetch modeling doesn't work for selective scheduling, it uses haifa
>> structures that are not kept up to date during sel-sched.  So this is not
>> supposed to work as soon as the param value for prefetcher lookahead depth
>> is positive.
>>
>> The following patch works for me.  Could you check it with your testing? If
>> it works fine for you, I would install the patch both for trunk and gcc-5.
>> It would be great to force sel-sched to be enabled, too.  I could do that
>> but I don't have the hardware or cross-arm target tools at the moment.
>>
>>          * haifa-sched.c (autopref_multipass_dfa_lookahead_guard): Disable
>> for selective scheduler.
>>
> It does work for me, it also fixes the other ICE I reported (on pr69307).
>
> But note that both tests pass on trunk.

This looks to me like PR rtl-optimization/68236 which I fixed on trunk.

Kyrill

> Christophe
>
>
>> Best,
>> Andrey
>>
>>
>>> Christophe
>>>
>>>> Andrey
>>>>
>>>>> Andrey
>>>>
>>>>

Re: Various selective scheduling fixes

[Christophe Lyon]

On 1 April 2016 at 15:12, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote:
> Hi Christophe, Andrey,
>
>
> On 01/04/16 14:09, Christophe Lyon wrote:
>>
>> On 1 April 2016 at 10:54, Andrey Belevantsev <abel@ispras.ru> wrote:
>>>
>>> Hi Christophe,
>>>
>>>
>>> On 01.04.2016 10:33, Christophe Lyon wrote:
>>>>
>>>> On 31 March 2016 at 16:43, Andrey Belevantsev <abel@ispras.ru> wrote:
>>>>>
>>>>> Hello,
>>>>>
>>>>> On 14.03.2016 12:10, Andrey Belevantsev wrote:
>>>>>>
>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> In this thread I will be posting the patches for the fixed selective
>>>>>> scheduling PRs (except the one that was already kindly checked in by
>>>>>> Jeff).
>>>>>>   The patches were tested both on x86-64 and ia64 with the following
>>>>>> combination: 1) the usual bootstrap/regtest, which only utilizes
>>>>>> sel-sched
>>>>>> on its own tests, made by default to run on arm/ppc/x86-64/ia64; 2)
>>>>>> the
>>>>>> bootstrap/regtest with the second scheduler forced to sel-sched; 3)
>>>>>> both
>>>>>> schedulers forced to sel-sched.  In all cases everything seemed to be
>>>>>> fine.
>>>>>>
>>>>>> Three of the PRs are regressions, the other two showed different
>>>>>> errors
>>>>>> across the variety of releases tested by submitters;  I think all of
>>>>>> them
>>>>>> are appropriate at this stage -- they do not touch anything outside of
>>>>>> selective scheduling except the first patch where a piece of code from
>>>>>> sched-deps.c needs to be refactored into a function to be called from
>>>>>> sel-sched.c.
>>>>>
>>>>>
>>>>>
>>>>> I've backported all regression PRs to gcc-5-branch after testing there
>>>>> again
>>>>> with selective scheduling force enabled: PRs 64411, 66660, 69032,
>>>>> 69102.
>>>>> The first one was not marked as a regression as such but the test for
>>>>> PR
>>>>> 70292, which is duplicate, works for me on gcc 5.1 thus making it a
>>>>> regression, too.
>>>>>
>>>> Hi,
>>>>
>>>> The backport for pr69102 shows that the new testcase fails to compile
>>>> (ICE)
>>>> when GCC is configured as:
>>>>
>>>> --target=arm-none-linux-gnueabihf --with-float=hard --with-mode=arm
>>>> --with-cpu=cortex-a15 --with-fpu=neon-vfpv4
>>>>
>>>>
>>>>
>>>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/compile/pr69102.c:
>>>> In function 'foo':
>>>>
>>>>
>>>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/compile/pr69102.c:21:1:
>>>> internal compiler error: Segmentation fault
>>>> 0xa64d15 crash_signal
>>>>          /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/toplev.c:383
>>>> 0xfa41d7 autopref_multipass_dfa_lookahead_guard(rtx_insn*, int)
>>>>          /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/haifa-sched.c:5752
>>>> 0xa31cd2 invoke_dfa_lookahead_guard
>>>>          /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:4212
>>>> 0xa31cd2 find_best_expr
>>>>          /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:4415
>>>> 0xa343fb fill_insns
>>>>          /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:5570
>>>> 0xa343fb schedule_on_fences
>>>>          /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7395
>>>> 0xa36010 sel_sched_region_2
>>>>          /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7533
>>>> 0xa36f2a sel_sched_region_1
>>>>          /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7575
>>>> 0xa36f2a sel_sched_region(int)
>>>>          /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7676
>>>> 0xa37589 run_selective_scheduling()
>>>>          /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7752
>>>> 0xa14aed rest_of_handle_sched2
>>>>          /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sched-rgn.c:3647
>>>> 0xa14aed execute
>>>>          /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sched-rgn.c:3791
>>>>
>>>> See
>>>>
>>>> http://people.linaro.org/~christophe.lyon/cross-validation/gcc/gcc-5-branch/234625/arm-none-linux-gnueabihf/diff-gcc-rh60-arm-none-linux-gnueabihf-arm-cortex-a15-neon-vfpv4.txt
>>>>
>>>> Can you have a look?
>>>
>>>
>>> That's because A15 is the only place which enables
>>> autopref_multipass_dfa_lookahead_guard as the DFA lookahead guard hook.
>>> But
>>> autoprefetch modeling doesn't work for selective scheduling, it uses
>>> haifa
>>> structures that are not kept up to date during sel-sched.  So this is not
>>> supposed to work as soon as the param value for prefetcher lookahead
>>> depth
>>> is positive.
>>>
>>> The following patch works for me.  Could you check it with your testing?
>>> If
>>> it works fine for you, I would install the patch both for trunk and
>>> gcc-5.
>>> It would be great to force sel-sched to be enabled, too.  I could do that
>>> but I don't have the hardware or cross-arm target tools at the moment.
>>>
>>>          * haifa-sched.c (autopref_multipass_dfa_lookahead_guard):
>>> Disable
>>> for selective scheduler.
>>>
>> It does work for me, it also fixes the other ICE I reported (on pr69307).
>>
>> But note that both tests pass on trunk.
>
>
> This looks to me like PR rtl-optimization/68236 which I fixed on trunk.
>
You are right: I've just checked that backporting your patch r230088 does
fix the problems in the gcc-5 branch.

Can I commit it ? I guess the question is for Jakub?


> Kyrill
>
>
>> Christophe
>>
>>
>>> Best,
>>> Andrey
>>>
>>>
>>>> Christophe
>>>>
>>>>> Andrey
>>>>>
>>>>>> Andrey
>>>>>
>>>>>
>>>>>
>

Re: Various selective scheduling fixes

[Jeff Law]

On 04/01/2016 07:26 AM, Christophe Lyon wrote:
> On 1 April 2016 at 15:12, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote:
>> Hi Christophe, Andrey,
>>
>>
>> On 01/04/16 14:09, Christophe Lyon wrote:
>>>
>>> On 1 April 2016 at 10:54, Andrey Belevantsev <abel@ispras.ru> wrote:
>>>>
>>>> Hi Christophe,
>>>>
>>>>
>>>> On 01.04.2016 10:33, Christophe Lyon wrote:
>>>>>
>>>>> On 31 March 2016 at 16:43, Andrey Belevantsev <abel@ispras.ru> wrote:
>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> On 14.03.2016 12:10, Andrey Belevantsev wrote:
>>>>>>>
>>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> In this thread I will be posting the patches for the fixed selective
>>>>>>> scheduling PRs (except the one that was already kindly checked in by
>>>>>>> Jeff).
>>>>>>>    The patches were tested both on x86-64 and ia64 with the following
>>>>>>> combination: 1) the usual bootstrap/regtest, which only utilizes
>>>>>>> sel-sched
>>>>>>> on its own tests, made by default to run on arm/ppc/x86-64/ia64; 2)
>>>>>>> the
>>>>>>> bootstrap/regtest with the second scheduler forced to sel-sched; 3)
>>>>>>> both
>>>>>>> schedulers forced to sel-sched.  In all cases everything seemed to be
>>>>>>> fine.
>>>>>>>
>>>>>>> Three of the PRs are regressions, the other two showed different
>>>>>>> errors
>>>>>>> across the variety of releases tested by submitters;  I think all of
>>>>>>> them
>>>>>>> are appropriate at this stage -- they do not touch anything outside of
>>>>>>> selective scheduling except the first patch where a piece of code from
>>>>>>> sched-deps.c needs to be refactored into a function to be called from
>>>>>>> sel-sched.c.
>>>>>>
>>>>>>
>>>>>>
>>>>>> I've backported all regression PRs to gcc-5-branch after testing there
>>>>>> again
>>>>>> with selective scheduling force enabled: PRs 64411, 66660, 69032,
>>>>>> 69102.
>>>>>> The first one was not marked as a regression as such but the test for
>>>>>> PR
>>>>>> 70292, which is duplicate, works for me on gcc 5.1 thus making it a
>>>>>> regression, too.
>>>>>>
>>>>> Hi,
>>>>>
>>>>> The backport for pr69102 shows that the new testcase fails to compile
>>>>> (ICE)
>>>>> when GCC is configured as:
>>>>>
>>>>> --target=arm-none-linux-gnueabihf --with-float=hard --with-mode=arm
>>>>> --with-cpu=cortex-a15 --with-fpu=neon-vfpv4
>>>>>
>>>>>
>>>>>
>>>>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/compile/pr69102.c:
>>>>> In function 'foo':
>>>>>
>>>>>
>>>>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/compile/pr69102.c:21:1:
>>>>> internal compiler error: Segmentation fault
>>>>> 0xa64d15 crash_signal
>>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/toplev.c:383
>>>>> 0xfa41d7 autopref_multipass_dfa_lookahead_guard(rtx_insn*, int)
>>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/haifa-sched.c:5752
>>>>> 0xa31cd2 invoke_dfa_lookahead_guard
>>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:4212
>>>>> 0xa31cd2 find_best_expr
>>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:4415
>>>>> 0xa343fb fill_insns
>>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:5570
>>>>> 0xa343fb schedule_on_fences
>>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7395
>>>>> 0xa36010 sel_sched_region_2
>>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7533
>>>>> 0xa36f2a sel_sched_region_1
>>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7575
>>>>> 0xa36f2a sel_sched_region(int)
>>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7676
>>>>> 0xa37589 run_selective_scheduling()
>>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7752
>>>>> 0xa14aed rest_of_handle_sched2
>>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sched-rgn.c:3647
>>>>> 0xa14aed execute
>>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sched-rgn.c:3791
>>>>>
>>>>> See
>>>>>
>>>>> http://people.linaro.org/~christophe.lyon/cross-validation/gcc/gcc-5-branch/234625/arm-none-linux-gnueabihf/diff-gcc-rh60-arm-none-linux-gnueabihf-arm-cortex-a15-neon-vfpv4.txt
>>>>>
>>>>> Can you have a look?
>>>>
>>>>
>>>> That's because A15 is the only place which enables
>>>> autopref_multipass_dfa_lookahead_guard as the DFA lookahead guard hook.
>>>> But
>>>> autoprefetch modeling doesn't work for selective scheduling, it uses
>>>> haifa
>>>> structures that are not kept up to date during sel-sched.  So this is not
>>>> supposed to work as soon as the param value for prefetcher lookahead
>>>> depth
>>>> is positive.
>>>>
>>>> The following patch works for me.  Could you check it with your testing?
>>>> If
>>>> it works fine for you, I would install the patch both for trunk and
>>>> gcc-5.
>>>> It would be great to force sel-sched to be enabled, too.  I could do that
>>>> but I don't have the hardware or cross-arm target tools at the moment.
>>>>
>>>>           * haifa-sched.c (autopref_multipass_dfa_lookahead_guard):
>>>> Disable
>>>> for selective scheduler.
>>>>
>>> It does work for me, it also fixes the other ICE I reported (on pr69307).
>>>
>>> But note that both tests pass on trunk.
>>
>>
>> This looks to me like PR rtl-optimization/68236 which I fixed on trunk.
>>
> You are right: I've just checked that backporting your patch r230088 does
> fix the problems in the gcc-5 branch.
>
> Can I commit it ? I guess the question is for Jakub?
Yes, you can commit it to the branch.

jeff

Re: Various selective scheduling fixes

[Christophe Lyon]

On 1 April 2016 at 18:19, Jeff Law <law@redhat.com> wrote:
> On 04/01/2016 07:26 AM, Christophe Lyon wrote:
>>
>> On 1 April 2016 at 15:12, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com>
>> wrote:
>>>
>>> Hi Christophe, Andrey,
>>>
>>>
>>> On 01/04/16 14:09, Christophe Lyon wrote:
>>>>
>>>>
>>>> On 1 April 2016 at 10:54, Andrey Belevantsev <abel@ispras.ru> wrote:
>>>>>
>>>>>
>>>>> Hi Christophe,
>>>>>
>>>>>
>>>>> On 01.04.2016 10:33, Christophe Lyon wrote:
>>>>>>
>>>>>>
>>>>>> On 31 March 2016 at 16:43, Andrey Belevantsev <abel@ispras.ru> wrote:
>>>>>>>
>>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> On 14.03.2016 12:10, Andrey Belevantsev wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> In this thread I will be posting the patches for the fixed selective
>>>>>>>> scheduling PRs (except the one that was already kindly checked in by
>>>>>>>> Jeff).
>>>>>>>>    The patches were tested both on x86-64 and ia64 with the
>>>>>>>> following
>>>>>>>> combination: 1) the usual bootstrap/regtest, which only utilizes
>>>>>>>> sel-sched
>>>>>>>> on its own tests, made by default to run on arm/ppc/x86-64/ia64; 2)
>>>>>>>> the
>>>>>>>> bootstrap/regtest with the second scheduler forced to sel-sched; 3)
>>>>>>>> both
>>>>>>>> schedulers forced to sel-sched.  In all cases everything seemed to
>>>>>>>> be
>>>>>>>> fine.
>>>>>>>>
>>>>>>>> Three of the PRs are regressions, the other two showed different
>>>>>>>> errors
>>>>>>>> across the variety of releases tested by submitters;  I think all of
>>>>>>>> them
>>>>>>>> are appropriate at this stage -- they do not touch anything outside
>>>>>>>> of
>>>>>>>> selective scheduling except the first patch where a piece of code
>>>>>>>> from
>>>>>>>> sched-deps.c needs to be refactored into a function to be called
>>>>>>>> from
>>>>>>>> sel-sched.c.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I've backported all regression PRs to gcc-5-branch after testing
>>>>>>> there
>>>>>>> again
>>>>>>> with selective scheduling force enabled: PRs 64411, 66660, 69032,
>>>>>>> 69102.
>>>>>>> The first one was not marked as a regression as such but the test for
>>>>>>> PR
>>>>>>> 70292, which is duplicate, works for me on gcc 5.1 thus making it a
>>>>>>> regression, too.
>>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> The backport for pr69102 shows that the new testcase fails to compile
>>>>>> (ICE)
>>>>>> when GCC is configured as:
>>>>>>
>>>>>> --target=arm-none-linux-gnueabihf --with-float=hard --with-mode=arm
>>>>>> --with-cpu=cortex-a15 --with-fpu=neon-vfpv4
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/compile/pr69102.c:
>>>>>> In function 'foo':
>>>>>>
>>>>>>
>>>>>>
>>>>>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/compile/pr69102.c:21:1:
>>>>>> internal compiler error: Segmentation fault
>>>>>> 0xa64d15 crash_signal
>>>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/toplev.c:383
>>>>>> 0xfa41d7 autopref_multipass_dfa_lookahead_guard(rtx_insn*, int)
>>>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/haifa-sched.c:5752
>>>>>> 0xa31cd2 invoke_dfa_lookahead_guard
>>>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:4212
>>>>>> 0xa31cd2 find_best_expr
>>>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:4415
>>>>>> 0xa343fb fill_insns
>>>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:5570
>>>>>> 0xa343fb schedule_on_fences
>>>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7395
>>>>>> 0xa36010 sel_sched_region_2
>>>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7533
>>>>>> 0xa36f2a sel_sched_region_1
>>>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7575
>>>>>> 0xa36f2a sel_sched_region(int)
>>>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7676
>>>>>> 0xa37589 run_selective_scheduling()
>>>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sel-sched.c:7752
>>>>>> 0xa14aed rest_of_handle_sched2
>>>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sched-rgn.c:3647
>>>>>> 0xa14aed execute
>>>>>>           /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/sched-rgn.c:3791
>>>>>>
>>>>>> See
>>>>>>
>>>>>>
>>>>>> http://people.linaro.org/~christophe.lyon/cross-validation/gcc/gcc-5-branch/234625/arm-none-linux-gnueabihf/diff-gcc-rh60-arm-none-linux-gnueabihf-arm-cortex-a15-neon-vfpv4.txt
>>>>>>
>>>>>> Can you have a look?
>>>>>
>>>>>
>>>>>
>>>>> That's because A15 is the only place which enables
>>>>> autopref_multipass_dfa_lookahead_guard as the DFA lookahead guard hook.
>>>>> But
>>>>> autoprefetch modeling doesn't work for selective scheduling, it uses
>>>>> haifa
>>>>> structures that are not kept up to date during sel-sched.  So this is
>>>>> not
>>>>> supposed to work as soon as the param value for prefetcher lookahead
>>>>> depth
>>>>> is positive.
>>>>>
>>>>> The following patch works for me.  Could you check it with your
>>>>> testing?
>>>>> If
>>>>> it works fine for you, I would install the patch both for trunk and
>>>>> gcc-5.
>>>>> It would be great to force sel-sched to be enabled, too.  I could do
>>>>> that
>>>>> but I don't have the hardware or cross-arm target tools at the moment.
>>>>>
>>>>>           * haifa-sched.c (autopref_multipass_dfa_lookahead_guard):
>>>>> Disable
>>>>> for selective scheduler.
>>>>>
>>>> It does work for me, it also fixes the other ICE I reported (on
>>>> pr69307).
>>>>
>>>> But note that both tests pass on trunk.
>>>
>>>
>>>
>>> This looks to me like PR rtl-optimization/68236 which I fixed on trunk.
>>>
>> You are right: I've just checked that backporting your patch r230088 does
>> fix the problems in the gcc-5 branch.
>>
>> Can I commit it ? I guess the question is for Jakub?
>
> Yes, you can commit it to the branch.
>

OK, done at r234680

Christophe

> jeff
>