-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Add mir-opt tests to track MIR quality. #110713
Conversation
(rustbot has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
d02df82
to
da7b3e1
Compare
☔ The latest upstream changes (presumably #107404) made this pull request unmergeable. Please resolve the merge conflicts. |
👍 I think this is a much better way to factor the MIR tests. We should clearly delineate the point of the various MIR tests we have, this has been a frequent point of confusion for me personally, and this is improving that.
rust/compiler/rustc_mir_transform/src/inline.rs Lines 53 to 54 in 3462f79
The fact that all the codegen tests use These tests are uncommonly fragile because they contain line and column numbers from the standard library implementation. If we start to accumulate tests along these lines and don't normalize or erase the line/column information from the tests we can expect any change to I'm actually not sure if the fact that we currently have line/column info is a bug, because looking carefully over the diff here, I see this:
so it looks like something is trying to help out here, but mostly it isn't. |
This looks good to me. It would be nice to have an option to also scrub the inlined locations -- especially for things from other crates -- but that's an existing thing that doesn't need to block adding and moving tests here. (And if they're too painful, we can always remove tests.) r? @scottmcm |
☀️ Test successful - checks-actions |
Finished benchmarking commit (253b727): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. |
cc @scottmcm @saethlin
If you have other ideas, please say so.