Skip to content
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

llvm: Add support for vectorcall (X86_VectorCall) convention #30567

Merged
merged 3 commits into from
Jan 17, 2016

Conversation

steffengy
Copy link
Contributor

Add support to use functions exported using vectorcall.
This essentially only allows to pass a new LLVM calling convention
from rust to LLVM.

extern "vectorcall" fn abc(param: c_void);

references

http://llvm.org/docs/doxygen/html/CallingConv_8h_source.html
https://msdn.microsoft.com/en-us/library/dn375768.aspx

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (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.

@nagisa
Copy link
Member

nagisa commented Dec 27, 2015

This is probably most important for MSVC interfacing.

@nrc
Copy link
Member

nrc commented Dec 28, 2015

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned nrc Dec 28, 2015
@brson
Copy link
Contributor

brson commented Dec 29, 2015

Are there specific APIs that need this? I do not like adding features just because they exist in LLVM. Let's not add calling conventions lightly.

cc @rust-lang/lang

@nagisa
Copy link
Member

nagisa commented Dec 29, 2015

There doesn’t seem to be many uses of this calling conventions in public APIs, but there’s some (e.g. skia) that use this CC for optimisation purposes.

@steffengy
Copy link
Contributor Author

I only know of PHP 7 (which is with what I'm fiddling around currently).
Since __vectorcall is relatively new, you also can expect that there's more
some potential use for this in the future.

The general problem is that rust first doesn't support all of LLVMs
calling conventions, which forces you to patch rust to still use it
and secondly you also can't easily bypass rust and tell LLVM directly
to use one of its own calling conventions.

I do not quite understand why @brson doesn't want (more) parity with LLVM here,
except if it'd cause any further problems, which adding a calling convention
that is of use to someone really shouldn't, or am I missing something there?

@retep998
Copy link
Member

I think that Rust should support any calling convention that can be reasonably used from C code. Things like vectorcall can be used by C code quite easily (at least on Windows with msvc), and are likely to be part of the public interface of libraries. As such, Rust should be proactive in supporting these calling conventions when it knows they exist, rather than waiting for people to get fed up with maintaining custom builds of Rust.

There are weird custom calling conventions for things like Haskell and stuff which can't really be used from C code and are unlikely to be in the public interface of a library. I recommend not supporting these, as there's very little reason to support them.

@nrc
Copy link
Member

nrc commented Dec 29, 2015

The reason (aiui) for not exposing everything llvm exposes is that we don't want to be tied to the llvm backend any more than we already are. We might want to support a C or gcc or custom backend in the future and every extra implementation detail we expose which ties us to llvm makes that harder. It seems reasonable to expose calling conventions which are supported by all major C compilers if there is a demand. I don't know if that is the case here.

@steffengy
Copy link
Contributor Author

I indeed agree that exposing everything llvm exposes is a bad idea
(for calling conventions @retep998 had a good argumentation).

However I can't quite agree with which are supported by all major C compilers
because that introduces the following problems:

  • It prevents single platform code (that's e.g. only for windows) to use specific calling conventions (which applies to this PR)
  • It prevents early support of "new" calling conventions on the long term
    (when only one of the major compilers hasn't introduced support for a calling convention yet)
  • __vectorcall is basically an extension of win64/X86_64_Win64, which is currently supported

@nrc
Copy link
Member

nrc commented Dec 30, 2015

cc @rust-lang/tools

@steffengy these are good points. I'm not sure where we stand on platform-specific stuff. This probably needs some discussion some where (tools/lang/core teams).

@Gankra
Copy link
Contributor

Gankra commented Dec 30, 2015

Reading the MSDN page linked and this one makes it fairly clear to me that this isn't an LLVM-ism at all, and should be safe/desirable to provide.

@brson
Copy link
Contributor

brson commented Jan 6, 2016

I do not quite understand why @brson doesn't want (more) parity with LLVM here,
except if it'd cause any further problems, which adding a calling convention
that is of use to someone really shouldn't, or am I missing something there?

Rust is its own language, not a high-level interface to LLVM. We've had many cases in the past where patches exposed LLVM functionality just because it was neat and we could. LLVM is a big backend. It does lots of stuff. The more we expose the harder it is to maintain, spec, and port to non-LLVM backends.

@steffengy
Copy link
Contributor Author

@brson
Generally that seems like the right approach, but not for features, like in this case, which aren't
LLVM specific at all.
Besides the already existing support in MSVC and LLVM, Intel would also be an example for a compiler which is planning on introducing it into their future compiler versions:

Instead users can use
__regcall calling convention which is a workaround for _vectorcall.
_vectorcall calling convention will introduced in a future version of the Intel® C++ Compiler.

https://software.intel.com/sites/default/files/Vectorcall_regcall_Demystified.pdf

And since, as I already mentioned,

__vectorcall is basically an extension of win64/X86_64_Win64, which is currently supported

I really do not see any reason not to add support in this case.

@Gankra
Copy link
Contributor

Gankra commented Jan 6, 2016

Ok so y'all are kinda talking past each other on the issue of specific vs general.

Hashing it out on IRC, the ultimate issue here is:

Who's going to use this?

We don't like adding features that literally no one is going to use "because we can", because that leads to rot. @steffengy you seem to imply that you're interested in using this for some problem, but you haven't explicitly stated that. Are you? @retep998 are there any systems you maintain that will benefit from this? Are there public interfaces that require this (@nagisa couldn't find any)?

The fact that "this exists and isn't tied to LLVM specifically" is an ok start, but if there isn't any strong motivating usecases today, it might make sense to hold off on this.

@retep998
Copy link
Member

retep998 commented Jan 6, 2016

DirectXMath.h has #define XM_CALLCONV __vectorcall
In the Windows 10.0.10586.0 SDK there are 1236 mentions of XM_CALLCONV according to grep. However all those headers appear to be C++ only and don't provide any important functionality other than fancy math which can be done in pure Rust equally well. Most of the functions are inline functions anyway.
As such I don't have any immediate need for vectorcall. Maybe in the future I might, but nothing for now.

@steffengy
Copy link
Contributor Author

you seem to imply that you're interested in using this for some problem, but you haven't explicitly stated that. Are you?

Yes I currently am (else this PR wouldn't exist)

I only know of PHP 7 (which is with what I'm fiddling around currently).
Since __vectorcall is relatively new, you also can expect that there's more
some potential use for this in the future.

Additionally I also found https://github.com/google/skia that seems to use it
somehow, which might not be of much relevance.

We don't like adding features that literally no one is going to use "because we can", because that leads to rot.

That does apply generally, but I do not really think that applies here,
since it doesn't require very extensive changes but only very minor changes,
which even reduce the difference to LLVM at this point.
In terms of maintenance when you support a new compiler backend for example
it will be as much trouble as every other MS-specific calling convention
(like Win64 which is essentially the same),
which makes it either not supported or an easy pass-through.

@ranma42
Copy link
Contributor

ranma42 commented Jan 6, 2016

Regarding maintenance, I was surprised to see no test case in this PR, but according to git grep apparently none of the tests contains "aapcs" either. I found a couple of tests for win64 and fastcall, but nothing exhaustive. Do we have automatically generated tests for calling conventions?

@alexcrichton
Copy link
Member

I personally very much agree with @brson's sentiments about not exposing everything LLVM does "just because", however I also agree with @gankro's assessment that this may not apply to this PR (e.g. it seems that vectorcall is both a prior name and has usage in other compilers).

In terms of of a public API exposed by the compiler itself, there's a few facets:

  • The name of this is basically instantly stabilized, so we would want to be very certain that this is the precise name we want for this calling convention. I'm a little worried about this and would be more comfortable having this be a nightly feature to start out personally.
  • All future backends to support all of Rust would need to support this calling convention. I'm not too worried about this as this is unlikely to be widely used by 90% of Rust code and the other complexities of writing a new backend are likely to far outweigh this. (e.g. if an alternate backend doesn't support this to start out, that seems ok)
  • We may want to make sure that this is relatively well supported in LLVM itself. For example would it be possible to have assurances that LLVM won't generate errors if a vectorcall function has a specific set of arguments? For example if LLVM generates an error with two f32 arguments but not with two f64 arguments (for whatever reason) that would probably be something we'd want to argue against. (note that I personally have no idea what the vectorcall convention is, so this question/concern may just be moot)

@ranma42 no we don't auto-generate tests for things like this, but I would personally like to see at least one usage of this in a test regardless.

Overall I would like to explore putting this ABI behind a feature gate, and then I would be quite comfortable landing it.

@retep998
Copy link
Member

@alexcrichton The vectorcall calling convention is basically equivalent to win64 except that it can pass larger types and vectors via the SSE registers.

@brson
Copy link
Contributor

brson commented Jan 12, 2016

Yes I currently am (else this PR wouldn't exist)

Perhaps you can say more about what you want to do with this calling convention.

It still seems to me that there are not public APIs that require Rust to implement vectorcall for compatibility. So the only reason I see to implement it is that it offers a compelling performance improvement in some case. Even if that is true, it does not necessarily suggest to me that 'vectorcall' is the way for Rust to achieve the same thing. A performance win really needs to be big for us to add an entirely new calling convention.

@steffengy
Copy link
Contributor Author

There probably some private API methods I'm using, but most actually are public,
which you pretty much always need when you're doing something more
than an extension which provides a hello_world function:

Source-Reference (limited to 2 results per file)
Definition of ZEND_FASTCALL (which is __vectorcall and used in all recent builds

Whether there's a huge performance win, I cannot say, but since the Intel performance
team is on board with PHP I atleast hope they aren't very keen on premature optimizations.
I'm also not aware of any benchmarks in that region, if that's what you were looking for.

@alexcrichton
Copy link
Member

@brson how do you feel about adding this behind a feature gate?

@steffengy could you also add a positive test which uses the ABI?

@nikomatsakis
Copy link
Contributor

@steffengy reading the PR, I had one question: from what I can tell, this ABI is specific to Windows? Do you know what LLVM will do on other platforms?

I would think that we probably want to just error out on non-Windows targets.

@steffengy
Copy link
Contributor Author

@nikomatsakis LLVM seems to work fine using vectorcall on other platforms, else the tests wouldnt pass. You're right though that vectorcall is used only by msvc at the moment.

@nikomatsakis
Copy link
Contributor

@steffengy

LLVM seems to work fine using vectorcall on other platforms, else the tests wouldnt pass

What concerned me is more that LLVM may be doing some random thing that is neither well specified nor particularly portable.

@steffengy
Copy link
Contributor Author

@nikomatsakis
It might be weird to use a windows calling convention on a different platform
(that's also why it probably won't really happen in any crate in the future),
but as long as declaration and call use the same declaration,
there shouldn't be any unexpected behavior.

It's basically the same with Win64 at the moment (which rust supports),
which would have the same implications.
Since there isn't any extra handling for Win64 that I'm aware of in that regard,
I guess it should be also fine in this case.

@nikomatsakis
Copy link
Contributor

@steffengy

there shouldn't be any unexpected behavior.

To be clear, this is mostly a theoretical concern and I'm not overly worried. But just to spell it out. What I was worried about is that people call into some C library that is declared with this calling convention. Then we switch from LLVM to some other backend. And we find out that it's a pain to reproduce the precise behavior that LLVM had in this new backend, because it is linux specific and doesn't support this windows-specific calling convention.

Basically, part of the argument for supporting a calling convention is that it is widespread on Win64, and hence whatever backend we choose will support it, which I think makes sense. But it's not true on linux, and hence by permitting it we are committing ourselves more to LLVM than we otherwise would. No?

@steffengy
Copy link
Contributor Author

hence by permitting it we are committing ourselves more to LLVM than we otherwise would. No?

If somebody would use a linux library which would indeed use this calling convention,
it might cause troubles with switching to another backend
(even though it's a highly improbable example).

This problem exists for every windows-specific calling convention though
for example Win64 (which is the "predecessor" of vectorcall),
which is currently supported, the same would apply.

So you are not really bound to LLVM, but to a backend which supports windows-specific calling conventions, if someone would for whatever reason would want to use this convention on a non windows target (atleast today thats highly unlikely)

I also think it's more like a theoretical concern, which should not have any larger impact in the future.

@brson
Copy link
Contributor

brson commented Jan 16, 2016

Thanks for the examples @steffengy.

I reluctantly concede that it looks inevitable that we'll have to implement this. Go for it.

@alexcrichton
Copy link
Member

@bors: r+ 4396a2d

@bors
Copy link
Contributor

bors commented Jan 16, 2016

⌛ Testing commit 4396a2d with merge 400bacf...

@bors
Copy link
Contributor

bors commented Jan 16, 2016

💔 Test failed - auto-linux-64-nopt-t

@nagisa
Copy link
Member

nagisa commented Jan 16, 2016

@bors retry

bors added a commit that referenced this pull request Jan 16, 2016
Add support to use functions exported using vectorcall.
This essentially only allows to pass a new LLVM calling convention
from rust to LLVM.

```rust
extern "vectorcall" fn abc(param: c_void);
```

references
----
http://llvm.org/docs/doxygen/html/CallingConv_8h_source.html
https://msdn.microsoft.com/en-us/library/dn375768.aspx
@bors
Copy link
Contributor

bors commented Jan 16, 2016

⌛ Testing commit 4396a2d with merge 077f4ee...

@bors
Copy link
Contributor

bors commented Jan 17, 2016

💔 Test failed - auto-linux-64-nopt-t

@nagisa
Copy link
Member

nagisa commented Jan 17, 2016

@bors retry

@retep998
Copy link
Member

Wow, the linux version of rust is quite flaky with those spurious failures.

@bors bors merged commit 4396a2d into rust-lang:master Jan 17, 2016
@nikomatsakis
Copy link
Contributor

@steffengy

This problem exists for every windows-specific calling convention though for example Win64 (which is the "predecessor" of vectorcall), which is currently supported, the same would apply.

Yes. I think we should consider generating an error (or at least a warning) for all such ABIs when they are used outside the target platform. I agree this is not specific to vectorcall (nor to Windows; I'm sure there are linux-specific conventions out there) and didn't mean to imply otherwise. But, as you say, I'm not too worried about this being a big problem in practice. I'll just let it go. :)

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 3, 2024
…, r=ehuss

Add tracking issue and unstable book page for `"vectorcall"` ABI

Originally added in 2015 by rust-lang#30567, the Windows `"vectorcall"` ABI didn't have a tracking issue until now.

Tracking issue: rust-lang#124485
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 3, 2024
…, r=ehuss

Add tracking issue and unstable book page for `"vectorcall"` ABI

Originally added in 2015 by rust-lang#30567, the Windows `"vectorcall"` ABI didn't have a tracking issue until now.

Tracking issue: rust-lang#124485
Noratrieb added a commit to Noratrieb/rust that referenced this pull request Jun 4, 2024
…, r=ehuss

Add tracking issue and unstable book page for `"vectorcall"` ABI

Originally added in 2015 by rust-lang#30567, the Windows `"vectorcall"` ABI didn't have a tracking issue until now.

Tracking issue: rust-lang#124485
Noratrieb added a commit to Noratrieb/rust that referenced this pull request Jun 4, 2024
…, r=ehuss

Add tracking issue and unstable book page for `"vectorcall"` ABI

Originally added in 2015 by rust-lang#30567, the Windows `"vectorcall"` ABI didn't have a tracking issue until now.

Tracking issue: rust-lang#124485
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Jun 4, 2024
…, r=ehuss

Add tracking issue and unstable book page for `"vectorcall"` ABI

Originally added in 2015 by rust-lang#30567, the Windows `"vectorcall"` ABI didn't have a tracking issue until now.

Tracking issue: rust-lang#124485
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2024
Rollup merge of rust-lang#124486 - beetrees:vectorcall-tracking-issue, r=ehuss

Add tracking issue and unstable book page for `"vectorcall"` ABI

Originally added in 2015 by rust-lang#30567, the Windows `"vectorcall"` ABI didn't have a tracking issue until now.

Tracking issue: rust-lang#124485
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.