Fix SROA miscompile in large functions #46819
Conversation
I'm pretty sure at least - the trace in that issue looks basically identical to what I was seeing. Unfortunately, the test case no longer seems to reproduce the issue on master (which I'm unsurprised by - this error is very sensitive to the exact order of things). |
|
@maleadt was there an unreduced version of this that might result in a still-broken reproducer? |
During the development of #46775, we saw a miscompile in the compiler, specifically, we saw SROA failing to provide a proper PHI nest. The root cause of this turned out to be an incorrect dominance query. In particular, during incremental compaction, the non-compacted portion of the IncrementalCompact cfg is allocated, but not valid, so we must not query it for basic block information. Unfortunately, I don't really have a good test case for this. This code has grown mostly organically for a few years, and I think it's probably overdue for an overhaul.
| function bb_ordering() | ||
| lt=(<=) | ||
| by=x->first(x.stmts) | ||
| ord(lt, by, nothing, Forward) | ||
| end |
There was a problem hiding this comment.
How about making this constant?
| function bb_ordering() | |
| lt=(<=) | |
| by=x->first(x.stmts) | |
| ord(lt, by, nothing, Forward) | |
| end | |
| const BB_ORDERING = ord((<=), x->first(x.stmts), nothing, Forward) |
aviatesk
force-pushed
the
kf/nastysroabug
branch
from
f9d3e16
to
6068525
Compare
|
Thanks so much for fixing this. It looks like this PR fixes the problem we saw on #46775. |
|
@nanosoldier |
|
Your benchmark job has completed - no performance regressions were detected. A full report can be found here. |
I didn't keep any intermediaries, but IIRC I started from the |
|
Seems reasonable, thanks for getting to this Keno! I'd still like to recreate the dominance error just to make sure this actually fixes it, but it seems like it should. |
|
This needs a manual backport to 1.8.3 |
During the development of #46775, we saw a miscompile in the compiler, specifically, we saw SROA failing to provide a proper PHI nest. The root cause of this turned out to be an incorrect dominance query. In particular, during incremental compaction, the non-compacted portion of the IncrementalCompact cfg is allocated, but not valid, so we must not query it for basic block information.
Fixes #46408