* [Bug rtl-optimization/104869] [12 Regression] Miscompilation of qt5-qtdeclarative since r12-6342-ge7a7dbb5ca5dd696
2022-03-10 18:43 [Bug rtl-optimization/104869] New: [12 Regression] Miscompilation of qt5-qtdeclarative jakub at gcc dot gnu.org
@ 2022-03-10 19:32 ` jakub at gcc dot gnu.org
2022-03-10 19:32 ` jakub at gcc dot gnu.org
` (6 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: jakub at gcc dot gnu.org @ 2022-03-10 19:32 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104869
Jakub Jelinek <jakub at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |liuhongt at gcc dot gnu.org,
| |rsandifo at gcc dot gnu.org
Summary|[12 Regression] |[12 Regression]
|Miscompilation of |Miscompilation of
|qt5-qtdeclarative |qt5-qtdeclarative since
| |r12-6342-ge7a7dbb5ca5dd696
--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Bisected to r12-6342-ge7a7dbb5ca5dd69689f1a462ba7620180acfe8b0
Assembly difference is:
--- qv4codegen.s.r12-6341 2022-03-10 20:27:09.392500655 +0100
+++ qv4codegen.s.r12-6342 2022-03-10 20:23:00.256971680 +0100
@@ -52895,9 +52895,11 @@ _ZN3QV48Compiler7Codegen5visitEPN6QQmlJS
ld 2,24(1)
mr 9,3
mr 3,31
- cmpdi 4,9,0
- beq 4,.L7388
+ cmpdi 0,9,0
+ beq 0,.L7388
+ li 9,0
mr 31,4
+ cmpdi 4,9,0
.L7389:
ld 3,32(1)
lwz 9,0(3)
@@ -53428,9 +53430,11 @@ _ZN3QV48Compiler7Codegen5visitEPN6QQmlJS
ld 2,24(1)
mr 9,3
mr 3,31
- cmpdi 4,9,0
- beq 4,.L7477
+ cmpdi 0,9,0
+ beq 0,.L7477
+ li 9,0
mr 31,4
+ cmpdi 4,9,0
.L7478:
ld 3,32(1)
lwz 9,0(3)
That said, I suspect that the r12-6342 change just uncovered a latent fwprop or
rtl-ssa bug, because the commit didn't do anything with propagation across
clobbers.
^ permalink raw reply [flat|nested] 9+ messages in thread* [Bug rtl-optimization/104869] [12 Regression] Miscompilation of qt5-qtdeclarative since r12-6342-ge7a7dbb5ca5dd696
2022-03-10 18:43 [Bug rtl-optimization/104869] New: [12 Regression] Miscompilation of qt5-qtdeclarative jakub at gcc dot gnu.org
2022-03-10 19:32 ` [Bug rtl-optimization/104869] [12 Regression] Miscompilation of qt5-qtdeclarative since r12-6342-ge7a7dbb5ca5dd696 jakub at gcc dot gnu.org
@ 2022-03-10 19:32 ` jakub at gcc dot gnu.org
2022-03-11 11:00 ` jakub at gcc dot gnu.org
` (5 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: jakub at gcc dot gnu.org @ 2022-03-10 19:32 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104869
Jakub Jelinek <jakub at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Target| |powerpc64le-linux
Target Milestone|--- |12.0
Priority|P3 |P1
^ permalink raw reply [flat|nested] 9+ messages in thread* [Bug rtl-optimization/104869] [12 Regression] Miscompilation of qt5-qtdeclarative since r12-6342-ge7a7dbb5ca5dd696
2022-03-10 18:43 [Bug rtl-optimization/104869] New: [12 Regression] Miscompilation of qt5-qtdeclarative jakub at gcc dot gnu.org
2022-03-10 19:32 ` [Bug rtl-optimization/104869] [12 Regression] Miscompilation of qt5-qtdeclarative since r12-6342-ge7a7dbb5ca5dd696 jakub at gcc dot gnu.org
2022-03-10 19:32 ` jakub at gcc dot gnu.org
@ 2022-03-11 11:00 ` jakub at gcc dot gnu.org
2022-03-11 12:04 ` jakub at gcc dot gnu.org
` (4 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: jakub at gcc dot gnu.org @ 2022-03-11 11:00 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104869
Jakub Jelinek <jakub at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Last reconfirmed| |2022-03-11
Status|UNCONFIRMED |NEW
Ever confirmed|0 |1
--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Reduced self-contained testcase for the testsuite:
// PR rtl-optimization/104869
// { dg-do run }
// { dg-options "-O2 -fvisibility=hidden" }
// { dg-require-visibility "" }
struct QBasicAtomicInteger {
[[gnu::noipa]] int loadRelaxed() { return 1; }
};
struct RefCount {
bool deref() {
int count = atomic.loadRelaxed();
if (count)
return false;
return deref();
}
QBasicAtomicInteger atomic;
};
struct QArrayData {
RefCount ref;
};
struct QString {
~QString();
QArrayData d;
};
int ok;
QString::~QString() { d.ref.deref(); }
struct Label {
bool isValid() { return generator; }
int *generator;
int index;
};
struct ControlFlow;
struct Codegen {
[[gnu::noipa]] bool visit();
ControlFlow *controlFlow;
};
struct ControlFlow {
enum UnwindType { EE };
struct UnwindTarget {
Label linkLabel;
};
ControlFlow *parent;
UnwindType unwindTarget_type;
UnwindTarget unwindTarget() {
QString label;
ControlFlow *flow = this;
while (flow) {
Label l = getUnwindTarget(unwindTarget_type, label);
if (l.isValid())
return {l};
flow = flow->parent;
}
return UnwindTarget();
}
[[gnu::noipa]] Label getUnwindTarget(UnwindType, QString &) {
Label l = { &ok, 0 };
return l;
}
};
[[gnu::noipa]] void foo(int) {
ok = 1;
}
[[gnu::noipa]] bool Codegen::visit() {
if (!controlFlow)
return false;
ControlFlow::UnwindTarget target = controlFlow->unwindTarget();
if (target.linkLabel.isValid())
foo(2);
return false;
}
int
main() {
ControlFlow cf = { nullptr, ControlFlow::UnwindType::EE };
Codegen c = { &cf };
c.visit();
if (!ok)
__builtin_abort ();
}
^ permalink raw reply [flat|nested] 9+ messages in thread* [Bug rtl-optimization/104869] [12 Regression] Miscompilation of qt5-qtdeclarative since r12-6342-ge7a7dbb5ca5dd696
2022-03-10 18:43 [Bug rtl-optimization/104869] New: [12 Regression] Miscompilation of qt5-qtdeclarative jakub at gcc dot gnu.org
` (2 preceding siblings ...)
2022-03-11 11:00 ` jakub at gcc dot gnu.org
@ 2022-03-11 12:04 ` jakub at gcc dot gnu.org
2022-03-20 14:02 ` rsandifo at gcc dot gnu.org
` (3 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: jakub at gcc dot gnu.org @ 2022-03-11 12:04 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104869
--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I've tried to figure out where the former (GCC 10) all_uses_available_at
checking has gone into (i.e. where we actually check that the propagation is
possible, that something doesn't overwrite or clobber the src from the def_insn
in between the def_insn and use_insn), but I can't find it, must be hidden
somewhere.
Richard, could you please have a look or at least hint at where to look for it?
^ permalink raw reply [flat|nested] 9+ messages in thread* [Bug rtl-optimization/104869] [12 Regression] Miscompilation of qt5-qtdeclarative since r12-6342-ge7a7dbb5ca5dd696
2022-03-10 18:43 [Bug rtl-optimization/104869] New: [12 Regression] Miscompilation of qt5-qtdeclarative jakub at gcc dot gnu.org
` (3 preceding siblings ...)
2022-03-11 12:04 ` jakub at gcc dot gnu.org
@ 2022-03-20 14:02 ` rsandifo at gcc dot gnu.org
2022-03-21 10:52 ` cvs-commit at gcc dot gnu.org
` (2 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: rsandifo at gcc dot gnu.org @ 2022-03-20 14:02 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104869
rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |ASSIGNED
Assignee|unassigned at gcc dot gnu.org |rsandifo at gcc dot gnu.org
--- Comment #4 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
Mine. The problem is in function_info::make_use_available,
which doesn't cope correctly with cases in which the start
of the BB comes “between” two clobbers (in an RPO sense).
^ permalink raw reply [flat|nested] 9+ messages in thread* [Bug rtl-optimization/104869] [12 Regression] Miscompilation of qt5-qtdeclarative since r12-6342-ge7a7dbb5ca5dd696
2022-03-10 18:43 [Bug rtl-optimization/104869] New: [12 Regression] Miscompilation of qt5-qtdeclarative jakub at gcc dot gnu.org
` (4 preceding siblings ...)
2022-03-20 14:02 ` rsandifo at gcc dot gnu.org
@ 2022-03-21 10:52 ` cvs-commit at gcc dot gnu.org
2022-03-21 10:52 ` rsandifo at gcc dot gnu.org
2022-07-04 19:02 ` cvs-commit at gcc dot gnu.org
7 siblings, 0 replies; 9+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2022-03-21 10:52 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104869
--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The trunk branch has been updated by Richard Sandiford <rsandifo@gcc.gnu.org>:
https://gcc.gnu.org/g:4a3073f04e8b7987ad7bfe1bc23bfeb1d627ee6a
commit r12-7736-g4a3073f04e8b7987ad7bfe1bc23bfeb1d627ee6a
Author: Richard Sandiford <richard.sandiford@arm.com>
Date: Mon Mar 21 10:51:56 2022 +0000
rtl-ssa: Fix prev/next_def confusion [PR104869]
rtl-ssa chains definitions into an RPO list. It also groups
sequences of clobbers together into a single node, so that it's
possible to skip over the clobbers in constant time in order to
get the next or previous set.
When adding a clobber to an insn, the main DF barriers for that
clobber are the last use of the previous set (if any) and the next
set (if any); adding a new clobber to a sea of clobbers is fine.
def_lookup provided the basis for these barriers as prev_def ()
and next_def ().
But of course, in hindsight, those were bad names, since they
implied that the returned values were literally the previous
definition (of any kind) or the next definition (of any kind).
And function_info::make_use_available was using the same routines
assuming that they had that meaning. :-(
This made a difference for the case where the start of a BB
occurs in the middle of an (RPO) clobber group: we then want
the previous and next clobbers in the group, rather than the
set before the clobber group and the set after the clobber group.
This patch renames the existing routines to something that's hopefully
clearer (though also more long-winded). It then adds routines that
really do provide the previous and next definitions.
This complication is supposed to be internal to rtl-ssa and, as
mentioned above, is part of trying to reduce time complexity.
gcc/
PR middle-end/104869
* rtl-ssa/accesses.h (clobber_group::prev_clobber): Declare.
(clobber_group::next_clobber): Likewise.
(def_lookup::prev_def): Rename to...
(def_lookup::last_def_of_prev_group): ...this.
(def_lookup::next_def): Rename to...
(def_lookup::first_def_of_next_group): ...this.
(def_lookup::matching_or_prev_def): Rename to...
(def_lookup::matching_set_or_last_def_of_prev_group): ...this.
(def_lookup::matching_or_next_def): Rename to...
(def_lookup::matching_set_or_first_def_of_next_group): ...this.
(def_lookup::prev_def): New function, taking the lookup insn as
argument.
(def_lookup::next_def): Likewise.
* rtl-ssa/member-fns.inl (def_lookup::prev_def): Rename to...
(def_lookup::last_def_of_prev_group): ...this.
(def_lookup::next_def): Rename to...
(def_lookup::first_def_of_next_group): ...this.
(def_lookup::matching_or_prev_def): Rename to...
(def_lookup::matching_set_or_last_def_of_prev_group): ...this.
(def_lookup::matching_or_next_def): Rename to...
(def_lookup::matching_set_or_first_def_of_next_group): ...this.
* rtl-ssa/movement.h (restrict_movement_for_dead_range): Update
after
above renaming.
* rtl-ssa/accesses.cc (clobber_group::prev_clobber): New function.
(clobber_group::next_clobber): Likewise.
(def_lookup::prev_def): Likewise.
(def_lookup::next_def): Likewise.
(function_info::make_use_available): Pass the lookup insn to
def_lookup::prev_def and def_lookup::next_def.
gcc/testsuite/
PR middle-end/104869
* g++.dg/pr104869.C: New test.
^ permalink raw reply [flat|nested] 9+ messages in thread* [Bug rtl-optimization/104869] [12 Regression] Miscompilation of qt5-qtdeclarative since r12-6342-ge7a7dbb5ca5dd696
2022-03-10 18:43 [Bug rtl-optimization/104869] New: [12 Regression] Miscompilation of qt5-qtdeclarative jakub at gcc dot gnu.org
` (5 preceding siblings ...)
2022-03-21 10:52 ` cvs-commit at gcc dot gnu.org
@ 2022-03-21 10:52 ` rsandifo at gcc dot gnu.org
2022-07-04 19:02 ` cvs-commit at gcc dot gnu.org
7 siblings, 0 replies; 9+ messages in thread
From: rsandifo at gcc dot gnu.org @ 2022-03-21 10:52 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104869
rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Resolution|--- |FIXED
Status|ASSIGNED |RESOLVED
--- Comment #6 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
Fixed. Thanks for the reduction.
^ permalink raw reply [flat|nested] 9+ messages in thread* [Bug rtl-optimization/104869] [12 Regression] Miscompilation of qt5-qtdeclarative since r12-6342-ge7a7dbb5ca5dd696
2022-03-10 18:43 [Bug rtl-optimization/104869] New: [12 Regression] Miscompilation of qt5-qtdeclarative jakub at gcc dot gnu.org
` (6 preceding siblings ...)
2022-03-21 10:52 ` rsandifo at gcc dot gnu.org
@ 2022-07-04 19:02 ` cvs-commit at gcc dot gnu.org
7 siblings, 0 replies; 9+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2022-07-04 19:02 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104869
--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Richard Sandiford
<rsandifo@gcc.gnu.org>:
https://gcc.gnu.org/g:d6a8ac7e92dfe0a51d71525e212147d0b84f1224
commit r11-10110-gd6a8ac7e92dfe0a51d71525e212147d0b84f1224
Author: Richard Sandiford <richard.sandiford@arm.com>
Date: Mon Jul 4 20:02:26 2022 +0100
rtl-ssa: Fix prev/next_def confusion [PR104869]
rtl-ssa chains definitions into an RPO list. It also groups
sequences of clobbers together into a single node, so that it's
possible to skip over the clobbers in constant time in order to
get the next or previous set.
When adding a clobber to an insn, the main DF barriers for that
clobber are the last use of the previous set (if any) and the next
set (if any); adding a new clobber to a sea of clobbers is fine.
def_lookup provided the basis for these barriers as prev_def ()
and next_def ().
But of course, in hindsight, those were bad names, since they
implied that the returned values were literally the previous
definition (of any kind) or the next definition (of any kind).
And function_info::make_use_available was using the same routines
assuming that they had that meaning. :-(
This made a difference for the case where the start of a BB
occurs in the middle of an (RPO) clobber group: we then want
the previous and next clobbers in the group, rather than the
set before the clobber group and the set after the clobber group.
This patch renames the existing routines to something that's hopefully
clearer (though also more long-winded). It then adds routines that
really do provide the previous and next definitions.
This complication is supposed to be internal to rtl-ssa and, as
mentioned above, is part of trying to reduce time complexity.
gcc/
PR middle-end/104869
* rtl-ssa/accesses.h (clobber_group::prev_clobber): Declare.
(clobber_group::next_clobber): Likewise.
(def_lookup::prev_def): Rename to...
(def_lookup::last_def_of_prev_group): ...this.
(def_lookup::next_def): Rename to...
(def_lookup::first_def_of_next_group): ...this.
(def_lookup::matching_or_prev_def): Rename to...
(def_lookup::matching_set_or_last_def_of_prev_group): ...this.
(def_lookup::matching_or_next_def): Rename to...
(def_lookup::matching_set_or_first_def_of_next_group): ...this.
(def_lookup::prev_def): New function, taking the lookup insn as
argument.
(def_lookup::next_def): Likewise.
* rtl-ssa/member-fns.inl (def_lookup::prev_def): Rename to...
(def_lookup::last_def_of_prev_group): ...this.
(def_lookup::next_def): Rename to...
(def_lookup::first_def_of_next_group): ...this.
(def_lookup::matching_or_prev_def): Rename to...
(def_lookup::matching_set_or_last_def_of_prev_group): ...this.
(def_lookup::matching_or_next_def): Rename to...
(def_lookup::matching_set_or_first_def_of_next_group): ...this.
* rtl-ssa/movement.h (restrict_movement_for_dead_range): Update
after
above renaming.
* rtl-ssa/accesses.cc (clobber_group::prev_clobber): New function.
(clobber_group::next_clobber): Likewise.
(def_lookup::prev_def): Likewise.
(def_lookup::next_def): Likewise.
(function_info::make_use_available): Pass the lookup insn to
def_lookup::prev_def and def_lookup::next_def.
gcc/testsuite/
PR middle-end/104869
* g++.dg/pr104869.C: New test.
(cherry picked from commit 4a3073f04e8b7987ad7bfe1bc23bfeb1d627ee6a)
^ permalink raw reply [flat|nested] 9+ messages in thread