Add a target option "merge-functions", and a corresponding -Z flag (works around #57356)

[peterhj]

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 #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]

rust/src/librustc_codegen_ssa/back/write.rs

Line 155 in a36b960

self.merge_functions = sess.opts.optimize == config::OptLevel::Default ||

[2]

rust/src/librustc_codegen_llvm/llvm_util.rs

Line 64 in a36b960

add("-mergefunc-use-aliases");

[3] #56358
[4] #49479

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @zackmdavis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[peterhj]

r? @alexcrichton

[gnzlbg]

LGTM.

[gnzlbg]

LGTM.

[nagisa]

The target option could probably be forward looking and allow specifying any of merge-functions = disabled, merge-functions = trampolines and merge-functions = aliases.

[peterhj]

The latest commit adds "merge-functions" options for "aliases", "trampolines", and "disabled", as well as a matching -Z flag.

[nagisa]

It would also be nice to gain a -Z flag that controls merge-functions. I wanted it at least once for tests & it seems that it is applicable more widely than I initially expected.

MergeFunctions pass, which rustc currently enables by default at -O2 and -O3 [1], and extern "ptx-kernel" functions are currently not compatible with each other.

Does the NVPTX target, perchance, support the non-extern "ptx-kernel" functions at all? In that case this solution is not great as it would disable the optimisation (the trampoline version could still be used, right?) target-wide even for internal functions that use the compatible ABI.

[alexcrichton]

@bors: r+

This seems like a great start! We can always add more bells and whistles in the future too. I'd arguably say that this is an LLVM bug, but I'm fine fixing it on our end

[bors]

📌 Commit 4aba371 has been approved by alexcrichton

[nikic]

Yes, this is definitely an LLVM bug and it would be good to fix it there in the long term.

Would it be possible to do something similar to how weak functions are handled? I.e., when merging functions with the ptx-kernel CC, instead of simply calling one function from the other, we would instead create a new function (with default CC) and make both functions call the new function. Would NVPTX support this kind of scheme?

[nagisa]

Yes, this is definitely an LLVM bug

If we are landing this, then at least an LLVM issue should be filled & cross referenced to a corresponding Rust issue.

[peterhj]

It would also be nice to gain a -Z flag that controls merge-functions. I wanted it at least once for tests & it seems that it is applicable more widely than I initially expected.

The latest commit will add a -Z merge-functions as well, and both the -Z flag and the target option have the same options ("disabled", "trampolines", "aliases"). The -Z flag has precedence over the target option.

Does the NVPTX target, perchance, support the non-extern "ptx-kernel" functions at all? In that case this solution is not great as it would disable the optimisation (the trampoline version could still be used, right?) target-wide even for internal functions that use the compatible ABI.
...
Would it be possible to do something similar to how weak functions are handled? I.e., when merging functions with the ptx-kernel CC, instead of simply calling one function from the other, we would instead create a new function (with default CC) and make both functions call the new function. Would NVPTX support this kind of scheme?

Non-extern "ptx-kernel" functions are "device" functions which can call each other and be called by extern "ptx-kernel" functions. My observation though is that the NVPTX backend prefers to inline those functions rather than emit calls, so there should not be much fallout on device functions when disabling MergeFunctions. It definitely warrants some investigation along with the general LLVM issue for MergeFunctions+NVPTX and the potential fix.

[nagisa]

Use bug! instead of panic!.

[peterhj]

using a MergeFunctions enum now, so no more wildcard arm

[nagisa]

This is going to cause an ICE later when invalid values are provided as an argument, because argument parsing does not verify the valid values. Consider validating this in argument parsing.

rust/src/librustc/session/config.rs

Lines 941 to 948 in 2442823

	fn parse_panic_strategy(slot: &mut Option<PanicStrategy>, v: Option<&str>) -> bool {
	match v {
	Some("unwind") => *slot = Some(PanicStrategy::Unwind),
	Some("abort") => *slot = Some(PanicStrategy::Abort),
	_ => return false
	}
	true
	}

is a good example of how arguments should be handled.

[peterhj]

Fixed with a MergeFunctions enum

[nagisa]

@bors r+ Thanks!

[bors]

📌 Commit 3cd4013 has been approved by nagisa

[nikic]

I've reported https://bugs.llvm.org/show_bug.cgi?id=40232 upstream to track this issue.

[Centril]

@bors r=nagisa

[bors]

📌 Commit b91d211 has been approved by nagisa

[bors]

⌛️ Testing commit b91d211 with merge c87144f...