On 2021/12/17 09:30, Xionghu Luo via Gcc-patches wrote:
>
>
> 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
This is the last patch for the "cold bb in hot loop optimization" I'd
like commit soon if not other comments, to let it fully tested more
broadly before stage4. Thanks.
Regression tested pass on powerpc64le-linux-gnu {P10,P9},
powerpc64-linux-gnu {P8, P7} and X86 though.
>
> 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" } } */
--
Thanks,
Xionghu
