-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Emit warning when calling/declaring functions with unavailable vectors. #132173
base: master
Are you sure you want to change the base?
Conversation
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Emit warning when calling/declaring functions with unavailable vectors. On some architectures, vector types may have a different ABI depending on whether the relevant target features are enabled. (The ABI when the feature is disabled is often not specified, but LLVM implements some de-facto ABI.) As discussed in rust-lang/lang-team#235, this turns out to very easily lead to unsound code. This commit makes it a post-monomorphization future-incompat warning to declare or call functions using those vector types in a context in which the corresponding target features are disabled, if using an ABI for which the difference is relevant. This ensures that these functions are always called with a consistent ABI. See the [nomination comment](rust-lang#127731 (comment)) for more discussion. Part of rust-lang#116558 r? RalfJung
The collector always runs, so likely we'll have to make this new check a query to avoid the perf issues. For the declaration-site check this should be fairly easy, we can pass in the monomorphized instance and that has everything we need. The call-site check is more tricky since the inputs currently are |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (bbf9ed8): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 4.2%, secondary 3.1%)This 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.
CyclesResults (primary 11.1%, secondary 12.1%)This 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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 785.03s -> 787.479s (0.31%) |
This comment has been minimized.
This comment has been minimized.
You'll probably need to fix the compilation error to make it buildable, but yes |
@RalfJung: Why not just make the query something like |
I thought of doing the same too - I also gave up on the previous attempt since that got in a somewhat annoying rabbit hole. |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Emit warning when calling/declaring functions with unavailable vectors. On some architectures, vector types may have a different ABI depending on whether the relevant target features are enabled. (The ABI when the feature is disabled is often not specified, but LLVM implements some de-facto ABI.) As discussed in rust-lang/lang-team#235, this turns out to very easily lead to unsound code. This commit makes it a post-monomorphization future-incompat warning to declare or call functions using those vector types in a context in which the corresponding target features are disabled, if using an ABI for which the difference is relevant. This ensures that these functions are always called with a consistent ABI. See the [nomination comment](rust-lang#127731 (comment)) for more discussion. Part of rust-lang#116558 r? RalfJung
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (95e2c91): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 3.1%, secondary -0.8%)This 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.
CyclesResults (primary 3.2%, secondary 3.0%)This 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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 783.187s -> 786.602s (0.44%) |
I'm afraid I have no idea how to further reduce the overhead here, I don't know much about the performance characteristics of the query system. This should be a largely unchanged query and thus basically trivial to cache for incremental builds but there are many things about this I do not understand. @compiler-errors @nnethercote maybe one of you has an idea? |
The detailed results for cargo debug incr-unchanged show that the |
That depends on what that query does :-) |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Emit warning when calling/declaring functions with unavailable vectors. On some architectures, vector types may have a different ABI depending on whether the relevant target features are enabled. (The ABI when the feature is disabled is often not specified, but LLVM implements some de-facto ABI.) As discussed in rust-lang/lang-team#235, this turns out to very easily lead to unsound code. This commit makes it a post-monomorphization future-incompat warning to declare or call functions using those vector types in a context in which the corresponding target features are disabled, if using an ABI for which the difference is relevant. This ensures that these functions are always called with a consistent ABI. See the [nomination comment](rust-lang#127731 (comment)) for more discussion. Part of rust-lang#116558 r? RalfJung
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (81ef4c3): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 2.8%, secondary 2.7%)This 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.
CyclesResults (primary 5.0%, secondary 6.2%)This 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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 785.104s -> 786.374s (0.16%) |
Something has gone wrong somewhere -- in your earlier result, the new The latest run also has a huge increase in the number of Not sure which of these two runs to trust, but at least one seems quite bogus. |
On some architectures, vector types may have a different ABI depending on whether the relevant target features are enabled. (The ABI when the feature is disabled is often not specified, but LLVM implements some de-facto ABI.) As discussed in rust-lang/lang-team#235, this turns out to very easily lead to unsound code. This commit makes it a post-monomorphization future-incompat warning to declare or call functions using those vector types in a context in which the corresponding target features are disabled, if using an ABI for which the difference is relevant. This ensures that these functions are always called with a consistent ABI. See the [nomination comment](rust-lang#127731 (comment)) for more discussion. Part of rust-lang#116558
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit a0b7398 with merge b0fa643490dcecd6aaca72fbb8a69124265ef9ad... |
Emit warning when calling/declaring functions with unavailable vectors. On some architectures, vector types may have a different ABI depending on whether the relevant target features are enabled. (The ABI when the feature is disabled is often not specified, but LLVM implements some de-facto ABI.) As discussed in rust-lang/lang-team#235, this turns out to very easily lead to unsound code. This commit makes it a post-monomorphization future-incompat warning to declare or call functions using those vector types in a context in which the corresponding target features are disabled, if using an ABI for which the difference is relevant. This ensures that these functions are always called with a consistent ABI. See the [nomination comment](rust-lang#127731 (comment)) for more discussion. Part of rust-lang#116558 r? RalfJung
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (b0fa643): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 2.1%, secondary 2.3%)This 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.
CyclesResults (primary 3.1%, secondary -1.4%)This 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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 782.919s -> 785.593s (0.34%) |
On some architectures, vector types may have a different ABI depending on whether the relevant target features are enabled. (The ABI when the feature is disabled is often not specified, but LLVM implements some de-facto ABI.)
As discussed in rust-lang/lang-team#235, this turns out to very easily lead to unsound code.
This commit makes it a post-monomorphization future-incompat warning to declare or call functions using those vector types in a context in which the corresponding target features are disabled, if using an ABI for which the difference is relevant. This ensures that these functions are always called with a consistent ABI.
See the nomination comment for more discussion.
Part of #116558
r? RalfJung