On Sat, Aug 3, 2024 at 3:31 PM Jeff Law <jeffreya...@gmail.com> wrote:
>
>
>
> On 8/2/24 11:31 PM, Sam James wrote:
> > Andrew Pinski <quic_apin...@quicinc.com> writes:
> >
> >> Late_combine exposed this latent bug in split_live_ranges_for_shrink_wrap.
> >> What it did was copy-prop regno 151 from regno 119 from:
> >> ```
> >> (insn 2 264 3 2 (set (reg/f:DI 119 [ thisD.3697 ])
> >> (reg:DI 151)) "/app/example.cpp":19:13 70 {*movdi_aarch64}
> >> (expr_list:REG_DEAD (reg:DI 151)
> >> (nil)))
> >> ```
> >>
> >> into these insns:
> >> ```
> >> (debug_insn 147 146 148 5 (var_location:DI thisD.3727 (reg/f:DI 119 [
> >> thisD.3697 ])) "/app/example.cpp":21:5 -1
> >> (nil))
> >> ....
> >> (insn 167 166 168 7 (set (reg:DI 1 x1)
> >> (reg/f:DI 119 [ thisD.3697 ])) "/app/example.cpp":14:21 70
> >> {*movdi_aarch64}
> >> (nil))
> >> ```
> >>
> >> Both are valid things to do. The problem is
> >> split_live_ranges_for_shrink_wrap looks at the
> >> uses of reg 151 and with and without debugging reg 151 have a different
> >> usage in different BBs.
> >> The function is trying to find a splitting point for reg 151 and they are
> >> different. In the end
> >> this causes register allocation difference.
> >> The fix is for split_live_ranges_for_shrink_wrap to ignore uses that were
> >> in debug insns.
> >>
> >> Bootstrappped and tested on x86_64-linux-gnu with no regressions.
> >>
> >> PR rtl-optimization/116179
> >>
> >> gcc/ChangeLog:
> >>
> >> * ira.cc (split_live_ranges_for_shrink_wrap): For the uses loop,
> >> only look at non-debug insns.
> >>
> >> gcc/testsuite/ChangeLog:
> >>
> >> * g++.dg/torture/pr116179-1.C: New test.
> >>
> >> Signed-off-by: Andrew Pinski <quic_apin...@quicinc.com>
> >> ---
> >> gcc/ira.cc | 4 +++-
> >> gcc/testsuite/g++.dg/torture/pr116179-1.C | 26 +++++++++++++++++++++++
> >> 2 files changed, 29 insertions(+), 1 deletion(-)
> >> create mode 100644 gcc/testsuite/g++.dg/torture/pr116179-1.C
> >>
> >> diff --git a/gcc/ira.cc b/gcc/ira.cc
> >> index 5642aea3caa..5a04231f7bd 100644
> >> --- a/gcc/ira.cc
> >> +++ b/gcc/ira.cc
> >> @@ -5144,7 +5144,9 @@ split_live_ranges_for_shrink_wrap (void)
> >> use = DF_REF_NEXT_REG (use))
> >> {
> >> int ubbi = DF_REF_BB (use)->index;
> >> - if (bitmap_bit_p (reachable, ubbi))
> >> + /* Only non debug insn should not be taken into account. */
> >
> > Nit: maybe /* Only non-debug insns should be ignored. */ to remove the
> > double negative or /* Don't ignore non-debug insns. */.
> Andrew's comment looks slightly wrong. I think he just needs to take
> out the "not". "insn" should be plural as well. If he wanted to remove
> the reference to "non debug" we often use "real" to refer to that case
> (INSN, JUMP_INSN and CALL_INSNs).
>
>
>
> OK with some kind of comment fix along those lines.
I originally had the test using `!DEBUG_INSN_P` so the comment was
originally `Debug insn should not be taken into account.`.
When I changed the test to use `NONDEBUG_INSN_P`, I also changed the
comment but had messed it up; forgetting to remove the `not`.
Anyways in the end I went with `Only non debug insns should be taken
into account.` since that reflex the check.
Thanks,
Andrew
>
> Jeff
>
>
