IRA: Ignore debug insns for uses in split_live_ranges_for_shrink_wrap. [PR116179]
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gcc_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gcc_check--master-arm |
success
|
Test passed
|
| linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 |
success
|
Test passed
|
Commit Message
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_apinski@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
Comments
Andrew Pinski <quic_apinski@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_apinski@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. */.
(Not sure if I've confused myself with the negatives there)
> + if (NONDEBUG_INSN_P (DF_REF_INSN (use))
> + && bitmap_bit_p (reachable, ubbi))
> bitmap_set_bit (need_new, ubbi);
> }
> last_interesting_insn = insn;
> [...]
thanks,
sam
On 8/2/24 11:31 PM, Sam James wrote:
> Andrew Pinski <quic_apinski@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_apinski@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.
Jeff
On Sat, Aug 3, 2024 at 3:31 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
> On 8/2/24 11:31 PM, Sam James wrote:
> > Andrew Pinski <quic_apinski@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_apinski@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
>
>
@@ -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. */
+ if (NONDEBUG_INSN_P (DF_REF_INSN (use))
+ && bitmap_bit_p (reachable, ubbi))
bitmap_set_bit (need_new, ubbi);
}
last_interesting_insn = insn;
new file mode 100644
@@ -0,0 +1,26 @@
+/* { dg-additional-options "-fcompare-debug" } */
+
+/* PR rtl-optimization/116179 */
+
+struct g *b;
+struct g {};
+void *operator new(__SIZE_TYPE__, void *);
+enum c {};
+struct d : g{} * e;
+c f;
+struct h {
+ g *k() {
+ d *a;
+ c i;
+ if (a || i == 0 || i == 1)
+ if (e || f)
+ return 0;
+ return new (&j) d;
+ }
+ void n();
+ g j;
+};
+void h::n() {
+ for (g *l(b), *m(b); l; l = m, m = 0)
+ k();
+}