Re: Fix PR 83913
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)
--- gcc/testsuite/ChangeLog (revision 259230)
***
**
Re: Fix PR 83913
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
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
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
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;
+}
