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

Update simd.rs to Rust 1.58 #414

Closed
wants to merge 3 commits into from
Closed

Conversation

afetisov
Copy link
Contributor

Removed stabilized intrinsics from the list of missing functions, added hard error on removed __m64 intrinsics, removed unused feature flag.

Copy link
Contributor

@fw-immunant fw-immunant left a comment

Choose a reason for hiding this comment

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

This is much-appreciated maintenance--thank you! We generally need to audit our emitted code and see what has been stabilized and what the MSRV of transpiled code can now look like, and this helps toward that goal.

There are a couple small changes I'd like:

  • I'd rather not introduce use of failure::bail, changing the return type of implicit_vector_default. It would be preferable to keep this code better aligned with surrounding idioms: we have our own (unrelated) bail! macro elsewhere in the codebase, no other usages of failure::bail, and return types generally use Result<_, TranslationError>.
  • Could you clarify in your commit message that _mm_cvtph_ps is now implemented (cf. _mm_loadu_si64, which is now removed)?

@afetisov
Copy link
Contributor Author

@fw-immunant

Sorry for the long delay. I have decided to close this PR, since testing has uncovered a bug in the current support of SIMD intrinsics, which means that the feature flag still cannot be removed even on x86/x86_64.

With regards to failure and the error types, I would strongly want to move to anyhow-based error handling. The current TranslationError is largely equivalent to opaque errors, the other constructors are mostly unused. Specifically, of the 4 TranslationErrorKind variants

#[derive(Clone, Eq, PartialEq, Debug)]
pub enum TranslationErrorKind {
    Generic,

    // Not enough simd intrinsics are available in LLVM < 7
    OldLLVMSimd,

    // We are waiting for va_copy support to land in rustc
    VaCopyNotImplemented,

    // Clang AST exported by AST-exporter was not valid
    InvalidClangAst(ClangAstParseErrorKind),
}

only Generic and OldLLVMSimd are used anywhere in code. Of these, Generic is equivalent to anyhow::Error (or failure-based errors), while OldLLVMSimd is used in exactly 2 places which are easier to handle with a locally defined error type (if it should be handled at all, rather than just panic).

I don't know whether I should discuss it here, or open a separate issue.

@afetisov afetisov closed this Jun 19, 2022
@fw-immunant
Copy link
Contributor

Thanks for following up. Whenever the rustc bug gets fixed we should revive this push.

The error refactoring does sound like it would be worthwhile--I just want to keep things internally consistent, so we should address it as a unit across the codebase. An issue for that would also be great. Thanks for your time!

@kkysen
Copy link
Contributor

kkysen commented Jun 21, 2022

Regarding error handling, there are currently a lot of .unwrap()s and uses of the outdated failure, which is currently deprecated (it's in rust-lang-deprecated). I think it's definitely a good idea to move to anyhow, eyre/color-eyre, etc. I'm trying that first in the new, smaller crates we're working on now.

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.

3 participants