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 Vector Operations #940

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

novafacing
Copy link

@novafacing novafacing commented May 28, 2023

I discovered several issues when trying to transpile ggml.c (Attached along with a compile_commands.json) and decided to learn a little bit about c2rust and try and tackle them. I'm submitting as a draft PR for reference, I'm interested in tackling some of this myself but I'm not a member of the project so I wanted to start a discussion before putting in that effort. The issues I spotted are:

  • Many builtins added since Rust 1.29.0 are not present in translator/builtins.rs (I added the builtins I needed, but the list in std::core::(x86|x86_64) is now quite long and more intrinsics can be supported than when it looks like this was initially written)
  • Indexing vector types was not allowed. I've taken a stab at implementing this and it seems to work.
  • Unrelated to vector operations, use path::elem::X as Y; was difficult to access. I've added some functionality to the ItemStore and Builder [repro.zip](https://github.com/immunant/c2rust/files/11585897/repro.zip) to make it easier to emit this type of use statement.
  • Vector types like __v8hi are emitted when translating C builtins like _cvtss_sh but were undefined. I've added use as aliasing to define them, although this is unlikely to be correct in all cases.
  • The missing and x86 only SIMD lists were out of date.
  • Unaligned vector types were not supported. I've added use as aliasing for these too. I'm not sure if there is any case where it's incorrect to emit an aligned vector type where an unaligned type was before (although I suspect there may be some performance impact).
  • I've removed stripping of implicit casts of vector builtin parameters as I found it was incorrectly stripping cast levels that were previously correct (e.g. 0 as libc::c_uint as libc::c_int would become 0 as libc::c_uint when the parameter type was indeed libc::c_int). I couldn't find a case where this caused more errors than it fixed, although again this may be the wrong approach.
  • Added short packed values to the list of valid arrangements of vector integral type initializers.

There is one problem I didn't address which is that implicit casting between vector types doesn't seem to be supported anywhere. I couldn't figure out where I could access the "destination type" of these casts, but in general vector type cast builtins like _mm256_castps_pd can be emitted (these calls are free, they're just casts) to explicitly convert these types whenever an implicit cast is found.

@LegNeato
Copy link
Contributor

This looks like useful work, do you intend to take it out of draft?

@novafacing
Copy link
Author

I haven't heard any feedback on this draft (here or in discord), so unless someone from Immunant wants to chime in probably not. You are welcome to grab this code though if you'd like @LegNeato!

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