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

Automatic verification of MIPS MSA intrinsics #711

Merged
merged 4 commits into from
Apr 11, 2019

Conversation

gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented Apr 9, 2019

This PR adds automatic-verification of the MIPS MSA intrinsic type signatures

cc @rbirdic @wzssyqa

@gnzlbg gnzlbg requested a review from alexcrichton April 9, 2019 12:18
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

This looks great to me, thanks @gnzlbg!

The only general comment I'd have is that we may want to double check for a more official source for intrinsic definitions other than a gcc header file. I know at least on x86 the __builtin_* functions all don't always map well to Intel's own names (and often don't correspond to intrinsics). I know very little about MIPS SIMD though, so if gcc is more 1:1 with upstream vendors then seems fine by me!

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Apr 10, 2019

The problem is that the C headers that do not include the __builtin_ use C macros to wrap those, so we can't extract any type information from them, e.g., see: https://github.com/gcc-mirror/gcc/blob/master/gcc/config/mips/msa.h#L52

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Apr 10, 2019

The clang header does the same: https://clang.llvm.org/doxygen/msa_8h_source.html

@alexcrichton
Copy link
Member

Ah ok that at least means directly using the __builtin macros is probably a good idea! Seems fine by me

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Apr 10, 2019

FYI i tried to enable checking the instructions, but something appears to be broken in the code that parses the assert_instr macros. It only finds intrinsics in 3/100 intrinsics. I've tried a bit of code golf there, but so much has changed in syn in the last years that I'm not really sure if the approach is correct anymore.

@alexcrichton
Copy link
Member

Ah no worries, if you want to land this an open an issue I can follow-up later poking around to see if I can enable this

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Apr 10, 2019

I'll wait for @rbirdic to review the proposed changes to the msa module and merge afterwards.

Copy link
Contributor

@rbirdic rbirdic left a comment

Choose a reason for hiding this comment

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

@gnzlbg This looks good to me. Thank you!

@gnzlbg gnzlbg merged commit f4b5561 into rust-lang:master Apr 11, 2019
rbirdic added a commit to rbirdic/stdsimd that referenced this pull request Apr 15, 2019
gnzlbg pushed a commit that referenced this pull request Apr 15, 2019
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