-
Notifications
You must be signed in to change notification settings - Fork 289
Fix windows builds & add some travis windows builds #592
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
Conversation
@@ -1109,7 +1111,8 @@ pub unsafe fn _mm256_extractf128_pd(a: __m256d, imm8: i32) -> __m128d { | |||
/// [Intel's documentation](https://software.intel.com/sites/landingpage/IntrinsicsGuide/#text=_mm256_extractf128_si256) | |||
#[inline] | |||
#[target_feature(enable = "avx")] | |||
#[cfg_attr(test, assert_instr(vextractf128, imm8 = 1))] | |||
#[cfg_attr(all(test, not(target_os = "windows")), | |||
assert_instr(vextractf128, imm8 = 1))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm do you know why these tests need to be ignored on Windows? I haven't had a chance to dig in yet but this seems somewhat bad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are just getting different codegen on windows AFAICT. What was being generated looked "ok".
By ok I mean that the |
For example:
Here it appears that the vector register is just directly read to memory instead of using |
Hm so in theory that should have all been fixed by this which tries to ensure the ABI has the registers coming in by-value so LLVM doesn't try to do anything tricky with optimizing the fact that they're already in memory. It may be though that the ABI isn't what we want or that it was tweaked perhaps. In any case this seems fine by me, it seems highly unlikely anyway to have a Windows-specific bug here |
@alexcrichton do you have a codegen tests for that windows |
Ah not in rustc afaik, apart from the standard "if we specify this does LLVM not crash?" |
I don't have a windows machine to test this, but maybe On godbolt things just work without any ABI: https://rust.godbolt.org/z/Kkyzl2 - and they work better with |
Oh hey looks like this may work! -- https://rust.godbolt.org/z/GARM_K |
Oh I should mention the ABI isn't required, the problem is that the ABI for Windows (I think) differs from the ABI for Unix, where Unix passes more things in registers than Windows. The fact that the Windows default ABI passes more things via memory causes the codegen differences on Windows/Unix. The attempt with |
I've pushed a commit using it instead of |
I was working off this list -- https://github.com/rust-lang/rust/blob/36a50c29f6c5c386fba6ab685818755ac55152e5/src/librustc_target/spec/abi.rs#L53-L77 |
:( |
alas! |
so I've reverted the last commit :/ |
I've opened #594 to track this. |
i686-pc-windows-gnu
is still not working on travis, but everything else should be green.