Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix SROA miscompile in large functions #46819

Merged
merged 2 commits into from
Sep 20, 2022
Merged

Fix SROA miscompile in large functions #46819

merged 2 commits into from
Sep 20, 2022

Conversation

Keno
Copy link
Member

@Keno Keno commented Sep 18, 2022

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

@Keno Keno added the backport 1.8 Change should be backported to release-1.8 label Sep 18, 2022
@Keno
Copy link
Member Author

Keno commented Sep 18, 2022

Fixes #46408

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).

@Keno
Copy link
Member Author

Keno commented Sep 18, 2022

@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.
Comment on lines +31 to +35
function bb_ordering()
lt=(<=)
by=x->first(x.stmts)
ord(lt, by, nothing, Forward)
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about making this constant?

Suggested change
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
Copy link
Member

Thanks so much for fixing this. It looks like this PR fixes the problem we saw on #46775.

@aviatesk
Copy link
Member

@nanosoldier runbenchmarks("inference", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - no performance regressions were detected. A full report can be found here.

@aviatesk aviatesk added the don't squash Don't squash merge label Sep 19, 2022
@maleadt
Copy link
Member

maleadt commented Sep 19, 2022

@maleadt was there an unreduced version of this that might result in a still-broken reproducer?

I didn't keep any intermediaries, but IIRC I started from the arrays2datablock function in Gnuplot.jl (which is called a bunch from their test suite).

@ianatol
Copy link
Member

ianatol commented Sep 19, 2022

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.

@Keno Keno merged commit 67f994c into master Sep 20, 2022
@Keno Keno deleted the kf/nastysroabug branch September 20, 2022 23:11
@KristofferC KristofferC mentioned this pull request Sep 30, 2022
37 tasks
@KristofferC
Copy link
Member

This needs a manual backport to 1.8.3

@KristofferC KristofferC mentioned this pull request Nov 8, 2022
26 tasks
@KristofferC KristofferC mentioned this pull request Dec 27, 2022
10 tasks
@KristofferC KristofferC mentioned this pull request Feb 6, 2023
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.8 Change should be backported to release-1.8 don't squash Don't squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segfault/assertion failure/IR verification error due to non-dominated use of SSAValue
6 participants