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

CLMUL instruction set #320

Merged
merged 9 commits into from
Feb 18, 2018
Merged

CLMUL instruction set #320

merged 9 commits into from
Feb 18, 2018

Conversation

newpavlov
Copy link
Contributor

@newpavlov newpavlov commented Feb 10, 2018

Fixes #318

Depends on rust-lang/rust#48126.

@alexcrichton
Copy link
Member

alexcrichton commented Feb 10, 2018

Thanks! The verification program is reporting

  • intel cpuid pclmulqdq not in pclmul for _mm_clmulepi64_si128

Perhaps the feature in Rust itself could be called pclmulqdq?

@newpavlov
Copy link
Contributor Author

newpavlov commented Feb 10, 2018

We have 3 options for feature name:

  • clmul - shortest one, coherent with intrinsic name _mm_clmulepi64_si128, matches with instruction set name
  • pclmul - used by LLVM
  • pclmulqdq - longest one, matches with CPUID and instruction mnemonic.

I've used pclmul because it required only simple whitelisting, while other options will require more involved changes, which I am currently don't know how to implement.

@alexcrichton
Copy link
Member

So far I think our intention is to provide a direct mapping from what vendors (Intel in this case) says. So far everything (except bmi/bmi1) has aligned with LLVM but I think for new ones we'll want to align with Intel rather than LLVM.

@newpavlov
Copy link
Contributor Author

Can you please list necessary changes for adding pclmulqdq feature which will use pclmul LLVM feature under the hood? (I think it's better to discuss in the relevant Rust PR)

@alexcrichton
Copy link
Member

Sure! I'll comment on the Rust PR

@alexcrichton
Copy link
Member

@newpavlov I think these should be in nightly now, right? If so, want to rebase to kick CI?

@newpavlov
Copy link
Contributor Author

I will change imm8: u8 to imm8: i32 to be coherent with other intrinsics.

Is it worth to open an issue for discussing potential change of i32 to u8 or i8 for immediate 8 bits arguments? I understand i32 was chosen to be coherent with headers, but I personally don't think we should follow them in this case.

@alexcrichton
Copy link
Member

Nah this is the territory of the RFC, so if you strongly feel we should be diverging from the headers I'd recommend reading over the discussion on the RFC and leaving a comment if you still feel that way. For now though, yeah, let's stick to the headers here.

@newpavlov
Copy link
Contributor Author

Hm, not sure how to fix this. In some cases _mm_clmulepi64_si128(a, b, 0x00) gets compiled to pclmullqlqdq %xmm1,%xmm0 and in others to pclmulqdq $0x0, %xmm1, %xmm0 . IIUC both mnemonics are equivalent.

@alexcrichton
Copy link
Member

Ah no worries, wanna only run that test on Linux and leave a fixme?

@newpavlov
Copy link
Contributor Author

Ah, indeed I could've run OS specific tests. I think last commit should fix it.

@alexcrichton
Copy link
Member

Awesome, thanks!

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