Conversation
This comment has been minimized.
This comment has been minimized.
hanna-kruppe
left a comment
There was a problem hiding this comment.
Some nits about some of the updated tests, otherwise LGTM.
cc @rust-lang/compiler for the possibility of regressions from using aliases instead of thunks. Wouldn't mind someone rubberstamping that, otherwise r=me.
There was a problem hiding this comment.
I'm surprised we don't have this function already anywhere 🤔
There was a problem hiding this comment.
Somewhat surprisingly, there is only a single use of LLVM_RUSTLLVM apart from the one introduced here. I guess the need to know about this from Rust code never came up.
I could also change the code to check for LLVM 8.0 instead and save this function. I only went for the RUSTLLVM check here because this is based on a cherry-picked commit, but ultimately checking for 8.0 would achieve the same and save some churn down the line.
There was a problem hiding this comment.
Likewise (-C no-prepopulate-passes).
There was a problem hiding this comment.
I've changed the other test to use no-prepopulate-passes, but not sure on this one, as it explicitly specifies -O.
There was a problem hiding this comment.
I went ahead and changed this one as well, as I really don't see a reason for it to use -O.
There was a problem hiding this comment.
Oh, I missed the -O, but I agree it doesn't seem relevant to the test. Still, cc @alexcrichton in case they remember why they wrote the test that way.
There was a problem hiding this comment.
Hm I'm not sure! Me being lazy with the copy/paste is the most likely reason, seems fine to switch it now!
|
r=me after squashing if travis passes |
If the Rust LLVM fork is used, enable the -mergefunc-use-aliases flag, which will create aliases for merged functions, rather than inserting a call from one to the other. A number of codegen tests needed to be adjusted, because functions that previously fell below the thunk limit are now being merged. Merging is prevented either using -C no-prepopulate-passes, or by making the functions non-identical. I expect that this is going to break something, somewhere, because it isn't able to deal with aliases properly, but we won't find out until we try :) This fixes rust-lang#52651.
1bbf849 to
cbf7489
Compare
|
@bors r=rkruppe |
|
📌 Commit cbf7489 has been approved by |
|
@bors r- Rollup #56430 failed on
while building stage2 std artifacts for We've seen this error a couple of times in conjunction with mergefuncs already... |
name-anon-globals should always be run at the very end of the pass pipeline, as optimization passes (in particular mergefunc) may introduce new anonymous globals. I believe we did not run into this earlier because it requires the rather specific combination of a) mergefunc merging two weak functions b) compilation not using thinlto.
|
@bors delegate+ |
|
✌️ @nikic can now approve this pull request |
|
@bors r=rkruppe We were running name-anon-globals before optimization passes instead of after... That covers the anon globals we introduce ourselves, but not those added by mergefuncs. |
|
📌 Commit 850d2f1 has been approved by |
Enable -mergefunc-use-aliases If the Rust LLVM fork is used, enable the -mergefunc-use-aliases flag, which will create aliases for merged functions, rather than inserting a call from one to the other. A number of codegen tests needed to be adjusted, because functions that previously fell below the thunk limit are now being merged. Merging is prevented in various ways now. I expect that this is going to break something, somewhere, because it isn't able to deal with aliases properly, but we won't find out until we try :) This fixes #52651. r? @rkruppe
|
☀️ Test successful - status-appveyor, status-travis |
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
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
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
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
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
…, r=rkruppe Use -mergefunc-use-aliases for any LLVM >= 8 This functionality is not specific to Rust's LLVM, but any starting in LLVM 8.0, as noted in <rust-lang#56358 (comment)>. cc @nikic r? @rkruppe
…, r=rkruppe Use -mergefunc-use-aliases for any LLVM >= 8 This functionality is not specific to Rust's LLVM, but any starting in LLVM 8.0, as noted in <rust-lang#56358 (comment)>. cc @nikic r? @rkruppe
…, r=rkruppe Use -mergefunc-use-aliases for any LLVM >= 8 This functionality is not specific to Rust's LLVM, but any starting in LLVM 8.0, as noted in <rust-lang#56358 (comment)>. cc @nikic r? @rkruppe
…, r=rkruppe Use -mergefunc-use-aliases for any LLVM >= 8 This functionality is not specific to Rust's LLVM, but any starting in LLVM 8.0, as noted in <rust-lang#56358 (comment)>. cc @nikic r? @rkruppe
If the Rust LLVM fork is used, enable the -mergefunc-use-aliases
flag, which will create aliases for merged functions, rather than
inserting a call from one to the other.
A number of codegen tests needed to be adjusted, because functions
that previously fell below the thunk limit are now being merged.
Merging is prevented in various ways now.
I expect that this is going to break something, somewhere, because
it isn't able to deal with aliases properly, but we won't find out
until we try :)
This fixes #52651.
r? @rkruppe