Re: Fix PR 83913

2018-04-09 Thread Andrey Belevantsev 
On 06.04.2018 19:10, Alexander Monakov wrote:
> On Tue, 3 Apr 2018, Andrey Belevantsev wrote:
> 
>> Hello,
>>
>> This issue ended up being fixed the way different from described in the PR.
>>  We do not want to walk away from the invariant "zero SCHED_TIMES -- insn
>> is not scheduled" even for bookkeeping copies (testing showed it trips over
>> asserts designed to catch this).  Rather we choose merging exprs in the way
>> the larger sched-times wins.
> 
> My understanding is this is not a "complete" solution to the problem, and a
> chance for a similar blowup on some other testcase remains. Still, avoiding
> picking the minimum sched-times value should be a good mitigation.

Well, it's not much different with any other situation when we pose a limit
on pipelining with the sched-times values.  At least for now I can't think
of something better.

Adjusted the comment as per your suggestion and committed the attached patch.

Andrey

> 
> Please consider adding a comment that the average sched-times value is taken
> as a compromise to thwart "endless" pipelining of bookkeeping-producing insns
> available anywhere vs. pipelining of useful insns, or something like that?
> 
> OK with that considered/added.
> 
>>
>> Best,
>> Andrey
>>
>> 2018-04-03  Andrey Belevantsev  
>>
>>  PR rtl-optimization/83913
>>
>>  * sel-sched-ir.c (merge_expr_data): Choose the middle between two
>> different sched-times
>>  when merging exprs.
>>
>>  * gcc.dg/pr83913.c: New test.
>>
>> diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
>> index a965d2ec42f..f6de96a7f3d 100644
>> --- a/gcc/sel-sched-ir.c
>> +++ b/gcc/sel-sched-ir.c
>> @@ -1837,8 +1837,9 @@ merge_expr_data (expr_t to, expr_t from, insn_t
>> split_point)
>>if (EXPR_PRIORITY (to) < EXPR_PRIORITY (from))
>>  EXPR_PRIORITY (to) = EXPR_PRIORITY (from);
>>
>> -  if (EXPR_SCHED_TIMES (to) > EXPR_SCHED_TIMES (from))
>> -EXPR_SCHED_TIMES (to) = EXPR_SCHED_TIMES (from);
>> +  if (EXPR_SCHED_TIMES (to) != EXPR_SCHED_TIMES (from))
>> +EXPR_SCHED_TIMES (to) = ((EXPR_SCHED_TIMES (from) + EXPR_SCHED_TIMES 
>> (to)
>> + + 1) / 2);
>>
>>if (EXPR_ORIG_BB_INDEX (to) != EXPR_ORIG_BB_INDEX (from))
>>  EXPR_ORIG_BB_INDEX (to) = 0;
>> @@ -3293,11 +3294,22 @@ has_dependence_note_mem_dep (rtx mem 
>> ATTRIBUTE_UNUSED,
>>
>>  /* Note a dependence.  */
>>  static void
>> diff --git a/gcc/testsuite/gcc.dg/pr83913.c b/gcc/testsuite/gcc.dg/pr83913.c
>> new file mode 100644
>> index 000..c898d71a261
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/pr83913.c
>> @@ -0,0 +1,26 @@
>> +/* { dg-do compile { target powerpc*-*-* ia64-*-* i?86-*-* x86_64-*-* } } */
>> +/* { dg-options "-O2 -funroll-all-loops -fselective-scheduling
>> -fsel-sched-pipelining -fschedule-insns -fno-dce -fno-forward-propagate
>> -fno-rerun-cse-after-loop -fno-web" } */
>> +
>> +int jo, z4;
>> +
>> +int
>> +be (long unsigned int l7, int nt)
>> +{
>> +  int en;
>> +
>> +  jo = l7;
>> +  for (en = 0; en < 24; ++en)
>> +{
>> +  jo = (jo / z4) * (!!jo >= ((!!nt) & 2));
>> +  if (jo == 0)
>> +nt = 0;
>> +  else
>> +{
>> +  nt = z4;
>> +  ++z4;
>> +  nt = (long unsigned int) nt == (l7 + 1);
>> +}
>> +}
>> +
>> +  return nt;
>> +}
>>
>>
>>

Index: gcc/ChangeLog
===
*** gcc/ChangeLog   (revision 259229)
--- gcc/ChangeLog   (revision 259230)
***
*** 1,5 
--- 1,12 
  2018-04-09  Andrey Belevantsev  
  
+   PR rtl-optimization/83913
+ 
+   * sel-sched-ir.c (merge_expr_data): Choose the middle between two
+   different sched-times when merging exprs.
+ 
+ 2018-04-09  Andrey Belevantsev  
+ 
PR rtl-optimization/83962
  
* sel-sched-ir.c (tidy_control_flow): Correct the order in which we call
Index: gcc/testsuite/gcc.dg/pr83913.c
===
*** gcc/testsuite/gcc.dg/pr83913.c  (revision 0)
--- gcc/testsuite/gcc.dg/pr83913.c  (revision 259230)
***
*** 0 
--- 1,26 
+ /* { dg-do compile { target powerpc*-*-* ia64-*-* i?86-*-* x86_64-*-* } } */
+ /* { dg-options "-O2 -funroll-all-loops -fselective-scheduling 
-fsel-sched-pipelining -fschedule-insns -fno-dce -fno-forward-propagate 
-fno-rerun-cse-after-loop -fno-web" } */
+ 
+ int jo, z4;
+ 
+ int
+ be (long unsigned int l7, int nt)
+ {
+   int en;
+ 
+   jo = l7;
+   for (en = 0; en < 24; ++en)
+ {
+   jo = (jo / z4) * (!!jo >= ((!!nt) & 2));
+   if (jo == 0)
+ nt = 0;
+   else
+ {
+   nt = z4;
+   ++z4;
+   nt = (long unsigned int) nt == (l7 + 1);
+ }
+ }
+ 
+   return nt;
+ }
Index: gcc/testsuite/ChangeLog
===
*** gcc/testsuite/ChangeLog (revision 259229)
---

Re: Fix PR 83913

2018-04-06 Thread Alexander Monakov 
On Tue, 3 Apr 2018, Andrey Belevantsev wrote:

> Hello,
> 
> This issue ended up being fixed the way different from described in the PR.
>  We do not want to walk away from the invariant "zero SCHED_TIMES -- insn
> is not scheduled" even for bookkeeping copies (testing showed it trips over
> asserts designed to catch this).  Rather we choose merging exprs in the way
> the larger sched-times wins.

My understanding is this is not a "complete" solution to the problem, and a
chance for a similar blowup on some other testcase remains. Still, avoiding
picking the minimum sched-times value should be a good mitigation.

Please consider adding a comment that the average sched-times value is taken
as a compromise to thwart "endless" pipelining of bookkeeping-producing insns
available anywhere vs. pipelining of useful insns, or something like that?

OK with that considered/added.

> 
> Best,
> Andrey
> 
> 2018-04-03  Andrey Belevantsev  
> 
>   PR rtl-optimization/83913
> 
>   * sel-sched-ir.c (merge_expr_data): Choose the middle between two
> different sched-times
>   when merging exprs.
> 
>   * gcc.dg/pr83913.c: New test.
> 
> diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
> index a965d2ec42f..f6de96a7f3d 100644
> --- a/gcc/sel-sched-ir.c
> +++ b/gcc/sel-sched-ir.c
> @@ -1837,8 +1837,9 @@ merge_expr_data (expr_t to, expr_t from, insn_t
> split_point)
>if (EXPR_PRIORITY (to) < EXPR_PRIORITY (from))
>  EXPR_PRIORITY (to) = EXPR_PRIORITY (from);
> 
> -  if (EXPR_SCHED_TIMES (to) > EXPR_SCHED_TIMES (from))
> -EXPR_SCHED_TIMES (to) = EXPR_SCHED_TIMES (from);
> +  if (EXPR_SCHED_TIMES (to) != EXPR_SCHED_TIMES (from))
> +EXPR_SCHED_TIMES (to) = ((EXPR_SCHED_TIMES (from) + EXPR_SCHED_TIMES (to)
> + + 1) / 2);
> 
>if (EXPR_ORIG_BB_INDEX (to) != EXPR_ORIG_BB_INDEX (from))
>  EXPR_ORIG_BB_INDEX (to) = 0;
> @@ -3293,11 +3294,22 @@ has_dependence_note_mem_dep (rtx mem ATTRIBUTE_UNUSED,
> 
>  /* Note a dependence.  */
>  static void
> diff --git a/gcc/testsuite/gcc.dg/pr83913.c b/gcc/testsuite/gcc.dg/pr83913.c
> new file mode 100644
> index 000..c898d71a261
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr83913.c
> @@ -0,0 +1,26 @@
> +/* { dg-do compile { target powerpc*-*-* ia64-*-* i?86-*-* x86_64-*-* } } */
> +/* { dg-options "-O2 -funroll-all-loops -fselective-scheduling
> -fsel-sched-pipelining -fschedule-insns -fno-dce -fno-forward-propagate
> -fno-rerun-cse-after-loop -fno-web" } */
> +
> +int jo, z4;
> +
> +int
> +be (long unsigned int l7, int nt)
> +{
> +  int en;
> +
> +  jo = l7;
> +  for (en = 0; en < 24; ++en)
> +{
> +  jo = (jo / z4) * (!!jo >= ((!!nt) & 2));
> +  if (jo == 0)
> +nt = 0;
> +  else
> +{
> +  nt = z4;
> +  ++z4;
> +  nt = (long unsigned int) nt == (l7 + 1);
> +}
> +}
> +
> +  return nt;
> +}
> 
> 
>

Re: Fix PR 83913

2018-04-03 Thread Andrey Belevantsev 
On 03.04.2018 19:02, Alexander Monakov wrote:
> On Tue, 3 Apr 2018, Andrey Belevantsev wrote:
> 
>> Hello,
>>
>> This issue ended up being fixed the way different from described in the PR.
>>  We do not want to walk away from the invariant "zero SCHED_TIMES -- insn
>> is not scheduled" even for bookkeeping copies (testing showed it trips over
>> asserts designed to catch this).  Rather we choose merging exprs in the way
>> the larger sched-times wins.
> 
> ... but the Changelog and the actual patch take the average rather than the
> maximum sched-time? :)  I believe either way would be acceptable, but please
> clarify the intent.

Sorry, the average is the intent.  Just to have a bit more of pipelining
chances.

Andrey

> 
> Alexander
>

Re: Fix PR 83913

2018-04-03 Thread Alexander Monakov 
On Tue, 3 Apr 2018, Andrey Belevantsev wrote:

> Hello,
> 
> This issue ended up being fixed the way different from described in the PR.
>  We do not want to walk away from the invariant "zero SCHED_TIMES -- insn
> is not scheduled" even for bookkeeping copies (testing showed it trips over
> asserts designed to catch this).  Rather we choose merging exprs in the way
> the larger sched-times wins.

... but the Changelog and the actual patch take the average rather than the
maximum sched-time? :)  I believe either way would be acceptable, but please
clarify the intent.

Alexander

Fix PR 83913

2018-04-03 Thread Andrey Belevantsev 
Hello,

This issue ended up being fixed the way different from described in the PR.
 We do not want to walk away from the invariant "zero SCHED_TIMES -- insn
is not scheduled" even for bookkeeping copies (testing showed it trips over
asserts designed to catch this).  Rather we choose merging exprs in the way
the larger sched-times wins.

Best,
Andrey

2018-04-03  Andrey Belevantsev  

PR rtl-optimization/83913

* sel-sched-ir.c (merge_expr_data): Choose the middle between two
different sched-times
when merging exprs.

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

diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
index a965d2ec42f..f6de96a7f3d 100644
--- a/gcc/sel-sched-ir.c
+++ b/gcc/sel-sched-ir.c
@@ -1837,8 +1837,9 @@ merge_expr_data (expr_t to, expr_t from, insn_t
split_point)
   if (EXPR_PRIORITY (to) < EXPR_PRIORITY (from))
 EXPR_PRIORITY (to) = EXPR_PRIORITY (from);

-  if (EXPR_SCHED_TIMES (to) > EXPR_SCHED_TIMES (from))
-EXPR_SCHED_TIMES (to) = EXPR_SCHED_TIMES (from);
+  if (EXPR_SCHED_TIMES (to) != EXPR_SCHED_TIMES (from))
+EXPR_SCHED_TIMES (to) = ((EXPR_SCHED_TIMES (from) + EXPR_SCHED_TIMES (to)
+ + 1) / 2);

   if (EXPR_ORIG_BB_INDEX (to) != EXPR_ORIG_BB_INDEX (from))
 EXPR_ORIG_BB_INDEX (to) = 0;
@@ -3293,11 +3294,22 @@ has_dependence_note_mem_dep (rtx mem ATTRIBUTE_UNUSED,

 /* Note a dependence.  */
 static void
diff --git a/gcc/testsuite/gcc.dg/pr83913.c b/gcc/testsuite/gcc.dg/pr83913.c
new file mode 100644
index 000..c898d71a261
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr83913.c
@@ -0,0 +1,26 @@
+/* { dg-do compile { target powerpc*-*-* ia64-*-* i?86-*-* x86_64-*-* } } */
+/* { dg-options "-O2 -funroll-all-loops -fselective-scheduling
-fsel-sched-pipelining -fschedule-insns -fno-dce -fno-forward-propagate
-fno-rerun-cse-after-loop -fno-web" } */
+
+int jo, z4;
+
+int
+be (long unsigned int l7, int nt)
+{
+  int en;
+
+  jo = l7;
+  for (en = 0; en < 24; ++en)
+{
+  jo = (jo / z4) * (!!jo >= ((!!nt) & 2));
+  if (jo == 0)
+nt = 0;
+  else
+{
+  nt = z4;
+  ++z4;
+  nt = (long unsigned int) nt == (l7 + 1);
+}
+}
+
+  return nt;
+}