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

Fix x86 compilation #376

Merged
merged 8 commits into from
Jul 4, 2024
Merged

Fix x86 compilation #376

merged 8 commits into from
Jul 4, 2024

Conversation

PigeonF
Copy link
Contributor

@PigeonF PigeonF commented Jun 26, 2024

Compiling for the i686-unknown-linux-gnu target leads to multiple compilation failures.

The first set was easy to solve (akin to #246). The second issue is that std::arch::x86_64::_mm_cvtsi128_si64 does not seem to have an equivalent for std::arch::x86. I think this is an oversight in std::arch::x86?

I am not confident enough in my SIMD knowledge to create a clever workaround, so I copied the "native rust implementation" for x86. If there is some workaround without using _mm_cvtsi128_si64, then feel free to change it.

Note

I do not have a x86 system myself, so I could not test the changes locally. I only checked by running cargo build --target i686-unknown-linux-gnu

@Licenser
Copy link
Member

nice catch and good solution. Using the native implementation is fine, 32-bit x86 systems are rare these days so I don't think it'll have a wide impact and with LLVM's optimisations it'll be plenty fast.

I suspect that the reason _mm_cvtsi128_si64 doesn't exist in x86 is that it returns a 64 bit value and the x86 cpu's don't have registers to fit that into nicely.

The arch alias is a nice addition too, I never thought of that but it's much cleaner, I learned some new trick here :) thanks!

There are a few issues with the tests since we got a new clippy lints, those are not related to your changes I think. I'm happy to fix those in the main branch and rebase your PR tomorrow if you don't want to deal with them.

CI aside, PR looks great 👍 thanks!

@PigeonF
Copy link
Contributor Author

PigeonF commented Jun 26, 2024

There is one more clippy lint that is reported on nightly: missing_transmute_annotations, specifically for

simd-json/src/macros.rs

Lines 1253 to 1259 in 83a806f

/// static cast to an i8
#[macro_export]
macro_rules! static_cast_i8 {
($v:expr) => {
::std::transmute::<_, i8>($v)
};
}

I am not sure if you want to just ignore the lint here or not, so I have not done anything about them. Properly fixing the lint would probably involve removing the macros and calling transmute directly instead, which seems like a whole lot of effort. On the other hand, I am not familiar with the code base, so I am not comfortable deciding that the lint should be ignored 😅

Comment on lines +152 to +153
# Legacy code
'cfg(portable)',
Copy link
Contributor Author

@PigeonF PigeonF Jun 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not too sure about the portable config (i.e. is the config supposed to be "hidden", or is this just remainder from previous code), so I have elected to ignore the warning about it here.

@@ -9,7 +9,7 @@ impl<'tape, 'input> PartialEq for Value<'tape, 'input> {
#[cfg_attr(not(feature = "no-inline"), inline)]
#[must_use]
fn eq(&self, other: &Self) -> bool {
self == other
self.0 == other.0
Copy link
Contributor Author

@PigeonF PigeonF Jun 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the previous implementation was buggy: the clippy lint indicates that it would have looped infinitely.

I think comparing the wrapped value is what was intended here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's right! Good catch :)

I stumbled about that the other day too, clippy is my secret hero to be honest 😂 it avoids so many issues.

@Licenser
Copy link
Member

Perfect :) thank you so much!

@Licenser Licenser merged commit ab9663d into simd-lite:main Jul 4, 2024
23 of 24 checks passed
@Licenser
Copy link
Member

Licenser commented Jul 4, 2024

Sorry it took me so long to merge this, I thought I had done it and must have forgotten.

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.

2 participants