Uh oh!
There was an error while loading. Please reload this page.
Rollup of 10 pull requests - #57752
Merged
Merged
Conversation
"trampolines", or "aliases (the default)) to allow targets to opt out of the MergeFunctions LLVM pass. Also add a corresponding -Z option with the same name and values. This works around: rust-lang#57356 Motivation: Basically, the problem is that the MergeFunctions pass, which rustc currently enables by default at -O2 and -O3, and `extern "ptx-kernel"` functions (specific to the NVPTX target) are currently not compatible with each other. If the MergeFunctions pass is allowed to run, rustc can generate invalid PTX assembly (i.e. a PTX file that is not accepted by the native PTX assembler ptxas). Therefore we would like a way to opt out of the MergeFunctions pass, which is what our target option does. Related work: The current behavior of rustc is to enable MergeFunctions at -O2 and -O3, and also to enable the use of function aliases within MergeFunctions. MergeFunctions both with and without function aliases is incompatible with the NVPTX target. clang's "solution" is to have a "-fmerge-functions" flag that opts in to the MergeFunctions pass, but it is not enabled by default.
In particular, I can use the following in my `config.toml`: ``` [build] host = ["i686-unknown-linux-gnu", "x86_64-unknown-linux-gnu"] target = ["i686-unknown-linux-gnu", "x86_64-unknown-linux-gnu"] ``` Before this change, my attempt to run the test suite would fail because the error output differs depending on what your host and targets are. ---- To be concrete, here are the actual messages one can observe: ``` % ./build/x86_64-unknown-linux-gnu/stage1/bin/rustc ../src/test/ui/huge-enum.rs -Aunused --target=x86_64-unknown-linux-gnu error: the type `std::option::Option<[u32; 35184372088831]>` is too big for the current architecture error: aborting due to previous error % ./build/x86_64-unknown-linux-gnu/stage1/bin/rustc ../src/test/ui/huge-enum.rs -Aunused --target=i686-unknown-linux-gnu error: the type `std::option::Option<[u32; 536870911]>` is too big for the current architecture error: aborting due to previous error % ./build/i686-unknown-linux-gnu/stage1/bin/rustc ../src/test/ui/huge-enum.rs -Aunused --target=i686-unknown-linux-gnu error: the type `std::option::Option<[u32; 536870911]>` is too big for the current architecture error: aborting due to previous error % ./build/i686-unknown-linux-gnu/stage1/bin/rustc ../src/test/ui/huge-enum.rs -Aunused --target=x86_64-unknown-linux-gnu error: the type `[u32; 35184372088831]` is too big for the current architecture error: aborting due to previous error ``` To address these variations, I changed the test to be more aggressive in its normalization strategy. We cannot (and IMO should not) guarantee that `Option` will appear in the error output here. So I normalized both types `Option<[u32; N]>` and `[u32; N]` to just `TYPE`
Point contributors to the rustc-guide...
Add a target option "merge-functions", and a corresponding -Z flag (works around rust-lang#57356) This commit adds a target option "merge-functions", which takes values in ("disabled", "trampolines", or "aliases" (default is "aliases")), to allow targets to opt out of the MergeFunctions LLVM pass. Additionally, the latest commit also adds an optional -Z flag, "merge-functions", which takes the same values and has precedence over the target option when both are specified. This works around rust-lang#57356. cc @eddyb@japaric@oli-obk@nox@nagisa Also thanks to @denzp and @gnzlbg for discussing this on rust-cuda! ### Motivation Basically, the problem is that the MergeFunctions pass, which rustc currently enables by default at -O2 and -O3 [1], and `extern "ptx-kernel"` functions (specific to the NVPTX target) are currently not compatible with each other. If the MergeFunctions pass is allowed to run, rustc can generate invalid PTX assembly (i.e. a PTX file that is not accepted by the native PTX assembler `ptxas`). Therefore we would like a way to opt out of the MergeFunctions pass, which is what our target option does. ### Related work The current behavior of rustc is to enable MergeFunctions at -O2 and -O3 [1], and also to enable the use of function aliases within MergeFunctions [2] [3]. MergeFunctions seems to have some benefits, such as reducing code size and fixing a crash [4], which is why it is enabled. However, MergeFunctions both with and without function aliases is incompatible with the NVPTX target; a more detailed example for both cases is given below. clang's "solution" is to have a "-fmerge-functions" flag that opts in to the MergeFunctions pass, but it is not enabled by default. ### Examples/more details Consider an example Rust lib using `extern "ptx-kernel"` functions: https://github.com/peterhj/nvptx-mergefunc-bug/blob/master/nocore.rs. If we try to compile this with nightly rustc, we get the following compiler error: LLVM ERROR: Module has aliases, which NVPTX does not support. This error happens because: (1) functions `foo` and `bar` have the same body, so are candidates to be merged by MergeFunctions; and (2) rustc configures MergeFunctions to generate function aliases using the "mergefunc-use-aliases" LLVM option [2] [3], but the NVPTX backend does not support those aliases. Okay, so we can try omitting "mergefunc-use-aliases", and then rustc will happily emit PTX assembly: https://github.com/peterhj/nvptx-mergefunc-bug/blob/master/nocore-mergefunc-nousealiases-bad.ptx. However, this PTX is invalid! When we try to assemble it with `ptxas` (I'm on the CUDA 9.2 toolchain), we get an assembler error: ptxas nocore-mergefunc-nousealiases-bad.ptx, line 38; error : Illegal call target, device function expected ptxas fatal : Ptx assembly aborted due to errors What's happening is that MergeFunctions rewrites the `bar` function to call `foo`. However, directly calling an `extern "ptx-kernel"` function from another `extern "ptx-kernel"` is wrong. If we disable the MergeFunctions pass from running at all, rustc generates correct PTX assembly: https://github.com/peterhj/nvptx-mergefunc-bug/blob/master/nocore-nomergefunc-ok.ptx [1] https://github.com/rust-lang/rust/blob/a36b960df626cbb8bea74f01243318b73f0bd201/src/librustc_codegen_ssa/back/write.rs#L155 [2] https://github.com/rust-lang/rust/blob/a36b960df626cbb8bea74f01243318b73f0bd201/src/librustc_codegen_llvm/llvm_util.rs#L64 [3] rust-lang#56358 [4] rust-lang#49479
Move glob map use to query and get rid of CrateAnalysis ~Also includes commits from ~rust-lang#57392 and ~rust-lang#57436 With glob map calculated unconditionally in rust-lang#57392, this PR moves the calculated glob map to `GlobalCtxt` and exposes a relevant query (as we do with other queries which copy precomputed data over from the `Resolver`). This allows us to get rid of the `CrateAnalysis` struct in an attempt to simplify the compiler interface. cc @Zoxc r? @nikomatsakis@Zoxc@petrochenkov
High priority resolutions for associated variants In rust-lang#56225 variants were assigned lowest priority during name resolution to avoid crater run and potential breakage. This PR changes the rules to give variants highest priority instead. Some motivation: - If variants (and their constructors) are treated as associated items, then they are obviously *inherent* associated items since they don't come from traits. - Inherent associated items have higher priority during resolution than associated items from traits. - The reason is that there is a way to disambiguate in favor of trait items (`<Type as Trait>::Ambiguous`), but there's no way to disambiguate in favor of inherent items, so they became unusable in case of ambiguities if they have low priority. - It's technically problematic to fallback from associated types to anything until lazy normalization (?) is implemented. Crater found some regressions from this change, but they are all in type positions, e.g. ```rust fn f() -> Self::Ambiguos { ... } // Variant `Ambiguous` or associated type `Ambiguous`? ``` , so variants are not usable there right now, but they may become usable in the future if rust-lang/rfcs#2593 is accepted. This PR keeps code like this successfully resolving, but introduces a future-compatibility lint `ambiguous_associated_items` that recommends rewriting it as `<Self as Trait>::Ambiguous`.
Querify `entry_fn` Analogous to rust-lang#57570 but this will also require few fixups in Miri so I decided to separate that (and it seems [CI doesn't let us break tools anymore](rust-lang#57392 (comment))? Or was that because it was a rollup PR?) r? @nikomatsakis
…enkov Fix nested `?` matchers fixrust-lang#57597 I'm not 100% if this works yet... cc @alercah When this is ready (but perhaps not yet):
…avidtwco Remove an unused function argument The only use was a debug printing, which might help someone with debugging dataflow problems, but seems otherwise useless
Make the contribution doc reference the guide more and also remove a lot of redundant info already in the guide
…to-work-cross-platform, r=nikomatsakis Generalize `huge-enum.rs` test and expected stderr for more cross platform cases With this change, I am able to build and test cross-platform `rustc` In particular, I can use the following in my `config.toml`: ``` [build] host = ["i686-unknown-linux-gnu", "x86_64-unknown-linux-gnu"] target = ["i686-unknown-linux-gnu", "x86_64-unknown-linux-gnu"] ``` Before this change, my attempt to run the test suite would fail because the error output differs depending on what your host and targets are. ---- To be concrete, here are the actual messages one can observe: ``` % ./build/x86_64-unknown-linux-gnu/stage1/bin/rustc ../src/test/ui/huge-enum.rs -Aunused --target=x86_64-unknown-linux-gnu error: the type `std::option::Option<[u32; 35184372088831]>` is too big for the current architecture error: aborting due to previous error % ./build/x86_64-unknown-linux-gnu/stage1/bin/rustc ../src/test/ui/huge-enum.rs -Aunused --target=i686-unknown-linux-gnu error: the type `std::option::Option<[u32; 536870911]>` is too big for the current architecture error: aborting due to previous error % ./build/i686-unknown-linux-gnu/stage1/bin/rustc ../src/test/ui/huge-enum.rs -Aunused --target=i686-unknown-linux-gnu error: the type `std::option::Option<[u32; 536870911]>` is too big for the current architecture error: aborting due to previous error % ./build/i686-unknown-linux-gnu/stage1/bin/rustc ../src/test/ui/huge-enum.rs -Aunused --target=x86_64-unknown-linux-gnu error: the type `[u32; 35184372088831]` is too big for the current architecture error: aborting due to previous error ``` To address these variations, I changed the test to be more aggressive in its normalization strategy. We cannot (and IMO should not) guarantee that `Option` will appear in the error output here. So I normalized both types `Option<[u32; N]>` and `[u32; N]` to just `TYPE`
Fix typo bug in DepGraph::try_mark_green(). r? @ZoxcFixesrust-lang#57692.
Update README.md Point contributors to the rustc-guide... r? @steveklabnik
Centril
commented
Jan 19, 2019
ContributorAuthor
@bors r+ p=10 |
bors
commented
Jan 19, 2019
Collaborator
📌 Commit f7b9084 has been approved by |
bors
commented
Jan 19, 2019
Collaborator
bors added a commit
that referenced
this pull request
Jan 19, 2019
Rollup of 10 pull requests Successful merges: - #57268 (Add a target option "merge-functions", and a corresponding -Z flag (works around #57356)) - #57476 (Move glob map use to query and get rid of CrateAnalysis) - #57501 (High priority resolutions for associated variants) - #57573 (Querify `entry_fn`) - #57610 (Fix nested `?` matchers) - #57634 (Remove an unused function argument) - #57653 (Make the contribution doc reference the guide more) - #57666 (Generalize `huge-enum.rs` test and expected stderr for more cross platform cases) - #57698 (Fix typo bug in DepGraph::try_mark_green().) - #57746 (Update README.md) Failed merges: r? @ghost
bors
commented
Jan 19, 2019
Collaborator
☀️ Test successful - checks-travis, status-appveyor |
rust-highfive
commented
Jan 19, 2019
Contributor
rust-highfive added a commit
to rust-lang-nursery/rust-toolstate
that referenced
this pull request
Jan 19, 2019
Tested on commit rust-lang/rust@c87144f. Direct link to PR: <rust-lang/rust#57752> 💔 miri on windows: test-pass → build-fail (cc @oli-obk@RalfJung@eddyb, @rust-lang/infra). 💔 miri on linux: test-pass → build-fail (cc @oli-obk@RalfJung@eddyb, @rust-lang/infra).
This was referenced Jan 19, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Successful merges:
entry_fn#57573 (Querifyentry_fn)?matchers #57610 (Fix nested?matchers)huge-enum.rstest and expected stderr for more cross platform cases #57666 (Generalizehuge-enum.rstest and expected stderr for more cross platform cases)Failed merges:
r? @ghost