On 2021/12/16 19:20, Jan Hubicka wrote:
>>
>> OK. Comments like?
>>
>> /* Don't move insn of cold BB out of loop to preheader to reduce calculations
>> and register live range in hot loop with cold BB. */
>
> Looks good.
>>
>>
>> And maybe some dump log will help tracking in xxx.c.271r.loop2_invariant.
>>
>> --- a/gcc/loop-invariant.c
>> +++ b/gcc/loop-invariant.c
>> @@ -1190,7 +1190,12 @@ find_invariants_bb (class loop *loop, basic_block bb,
>> bool always_reached,
>> basic_block preheader = loop_preheader_edge (loop)->src;
>>
>> if (preheader->count > bb->count)
>> - return;
>> + {
>> + if (dump_file)
>> + fprintf (dump_file, "Don't move invariant from bb: %d in loop %d\n",
>> + bb->index, loop->num);
>> + return;
>> + }
>
> This is also a good idea - testcases are quite important for this typo
> of stuff.
>>>
>>> Thinking about this more, you want to test:
>>> if (!always_executed && preheader->count > bb->count)
>>> return;
>>> This will rule out some profile misupates.
>>>
>>> Also the code updating always_reached is worng:
>>> if (always_reached
>>> && CALL_P (insn)
>>> && (RTL_LOOPING_CONST_OR_PURE_CALL_P (insn)
>>> || ! RTL_CONST_OR_PURE_CALL_P (insn)))
>>> always_reached = false;
>>> It misses the case where statement can trhow external exception or
>>> volatile asms that can also terminate execution.
>>>
>>> I bit worry on how often the test will have false positives with guessed
>>> profile where earlier loop optimizations reduced trip count below
>>> realistic estimate.
>>
>> Sorry I don't understand why always_executed and always_reached matters here?
>> the test is based on BB before the FOR_BB_INSNS loop...
>
> always_executed is useful here to avoid the situation where bb->count or
> preheader->count is wrong and it would disable wanted transformation.
>
> always_executed is just a bug I noticed while looking at the code. I
> will produce testcase for bugzilla.
>
> Honza
Thanks, so is this OK to commit now? And any additional comments for
the "[PATCH 3/3] Fix loop split incorrect count and probability"
(https://gcc.gnu.org/pipermail/gcc-patches/2021-December/586374.html)?
Updated comments and testcase:
[PATCH 1/3] loop-invariant: Don't move cold bb instructions to preheader in RTL
From: Xiong Hu Luo <luo...@linux.ibm.com>
gcc/ChangeLog:
* loop-invariant.c (find_invariants_bb): Check profile count
before motion.
(find_invariants_body): Add argument.
gcc/testsuite/ChangeLog:
* gcc.dg/loop-invariant-2.c: New.
---
gcc/loop-invariant.c | 17 ++++++++++++++---
gcc/testsuite/gcc.dg/loop-invariant-2.c | 20 ++++++++++++++++++++
2 files changed, 34 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/loop-invariant-2.c
diff --git a/gcc/loop-invariant.c b/gcc/loop-invariant.c
index fca0c2b24be..690f7704a0b 100644
--- a/gcc/loop-invariant.c
+++ b/gcc/loop-invariant.c
@@ -1183,9 +1183,21 @@ find_invariants_insn (rtx_insn *insn, bool
always_reached, bool always_executed)
call. */
static void
-find_invariants_bb (basic_block bb, bool always_reached, bool always_executed)
+find_invariants_bb (class loop *loop, basic_block bb, bool always_reached,
+ bool always_executed)
{
rtx_insn *insn;
+ basic_block preheader = loop_preheader_edge (loop)->src;
+
+ /* Don't move insn of cold BB out of loop to preheader to reduce calculations
+ and register live range in hot loop with cold BB. */
+ if (!always_executed && preheader->count > bb->count)
+ {
+ if (dump_file)
+ fprintf (dump_file, "Don't move invariant from bb: %d out of loop %d\n",
+ bb->index, loop->num);
+ return;
+ }
FOR_BB_INSNS (bb, insn)
{
@@ -1214,8 +1226,7 @@ find_invariants_body (class loop *loop, basic_block *body,
unsigned i;
for (i = 0; i < loop->num_nodes; i++)
- find_invariants_bb (body[i],
- bitmap_bit_p (always_reached, i),
+ find_invariants_bb (loop, body[i], bitmap_bit_p (always_reached, i),
bitmap_bit_p (always_executed, i));
}
diff --git a/gcc/testsuite/gcc.dg/loop-invariant-2.c
b/gcc/testsuite/gcc.dg/loop-invariant-2.c
new file mode 100644
index 00000000000..df3d8458569
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/loop-invariant-2.c
@@ -0,0 +1,20 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-rtl-loop2_invariant" } */
+
+volatile int x;
+void
+bar (int, char *, char *);
+void
+foo (int *a, int n, int k)
+{
+ int i;
+
+ for (i = 0; i < n; i++)
+ {
+ if (__builtin_expect (x, 0))
+ bar (k / 5, "one", "two");
+ a[i] = k;
+ }
+}
+
+/* { dg-final { scan-rtl-dump "Don't move invariant from bb: .*out of loop"
"loop2_invariant" } } */
--
2.27.0.90.geebb51ba8c
