Add bittest instructions for x86

[mehcode]

Closes #553

---

• I don't think the assert_instr lines are doing anything so not sure if I'm using them wrong or they just aren't applicable here. Ah. It does run on CI.

• I know the API differs slightly than what's prescribed in Add bittest intrinsics  #553 (comment) but after looking around this feels more consistent.

• Do we want to provide non-reference versions of bittest? Not sure if it'd be worth it.

• What's the policy on stuff like debug_asserts for checking invariants? If you test for a bit higher than the total bit size of the operand it runs right off the memory onto whatever is there at the moment.

[gnzlbg]

So the general approach looks good, and there are only some minor issues.

I don't think the assert_instr lines are doing anything so not sure if I'm using them wrong or they just aren't applicable here.

They only have an effect when building the tests in release mode (e.g. with cargo test --release).

---

The naming of the intrinsics should match the intel spec (https://software.intel.com/sites/landingpage/IntrinsicsGuide/#text=bittest), that is, no _u32, 64 instead of _u64, etc.

The spec uses mutable C pointers, and that makes little sense for an API that cannot read/write to uninitialized memory, doesn't accept null pointers, etc. So maybe this really is a case in which we should slightly deviate from the C spec and use reference types instead of *mut everywhere. cc @alexcrichton (that might require tuning the validation suite for these, but that's doable).

Also since LLVM does not have intrinsics for these and clang generates inline assembly as well, we might want to check that the LLVM-IR that we generate matches that of clang.

[Amanieu]

What are the semantics of these intrinsics when the bit index is larger than the word size? The BT* instructions will happily continue on the next word.

If this is intended behavior then we need to use the "memory" clobber.

[gnzlbg]

The BT* instructions will happily continue on the next word.

I just read the docs of the BT instructions and I understood those as the correct semantics.

The spec for the intrinsics says:

Return the bit at index b of 64-bit integer a.

which hints that one can't "index out of bounds". This might be a bug in the spec of the intrinsics, or might be the intended behavior, I don't know.

@mehcode I would be more comfortable if these would take a raw pointer - in case that turns out to be a bug on the spec of the intrinsics, the raw pointer would just work, while the reference API would be weird.

It would be great if somebody could ask for clarification here: https://software.intel.com/en-us/forums/intel-isa-extensions/topic/363747 (that's where spec bugs are reported).

[Amanieu]

Alternatively if you first load the value into a register and then run the BT* instruction on that register then the instruction will automatically mask the index to fit in the register size.

[gnzlbg]

Alternatively if you first load the value into a register and then run the BT* instruction on that register then the instruction will automatically mask the index to fit in the register size.

Sure. I was just wondering whether the intent of the C intrinsics is to limit usage of the bt instructions to this case, or whether the intent is to enable "full" usage of the bt instruction (I don't know the answer).

[Amanieu]

After reading some docs, I think the intended usage of this intrinsic is to expose the full BT instruction with an unlimited range. In this case the parameter must be changed to a raw pointer and the inline asm needs to use the "memory" clobber.

[gnzlbg]

After reading some docs,

Do you have a link?

[Amanieu]

https://software.intel.com/en-us/node/523362

Returns the bit in position b of the memory addressed by p.

It makes no mention of being limited to a single word, and I think the original intent of that intrinsic was to expose the BT instruction directly.

[alexcrichton]

@gnzlbg FWIW there's already a number of intrinsics that use &mut __m128 (example), and I don't think we've been too too consistent about the distinction between *mut and &mut historically. It should be fine to basically choose whatever's best as a local optimum

[mehcode]

Updated.

• Adjusted the constraints to indicate the read/write nature of the memory.
• Changed to raw pointers instead of references.

As this is meant to have access to a memory region and not be limited to a single primitive, a raw pointer makes sense to me.

[mehcode]

How do I fix the missing cpuid errors ?

failed to verify `_bittest`
  * missing cpuid for _bittest
failed to verify `_bittestandset`
  * missing cpuid for _bittestandset
failed to verify `_bittestandreset`
  * missing cpuid for _bittestandreset
failed to verify `_bittestandcomplement`
  * missing cpuid for _bittestandcomplement
failed to verify `_bittest64`
  * missing cpuid for _bittest64
failed to verify `_bittestandset64`
  * missing cpuid for _bittestandset64
failed to verify `_bittestandreset64`
  * missing cpuid for _bittestandreset64
failed to verify `_bittestandcomplement64`
  * missing cpuid for _bittestandcomplement64

[gnzlbg]

Is there a cpuid bit flag to detect these? (I don't think so?). Otherwise you need to whitelist them here: https://github.com/rust-lang-nursery/stdsimd/blob/master/crates/stdsimd-verify/tests/x86-intel.rs#L248

[gnzlbg]

@mehcode ping :)

[mehcode]

Build should be green now. I had to use i32 instead of u32 to pass the intrinsic verification test.

[gnzlbg]

Sorry that it took so long to review this again. Is there a PR for the simd_x86_bittest feature to rust-lang/rust ? Otherwise, I'd prefer to use the stdsimd feature here for now I think. Or what do you think @alexcrichton ? If we wanted to stabilize this in the near future, then adding their own feature flag might make more sense.

[alexcrichton]

Eh I think it'd be best to move away from stdsimd where we can, so I think landing this as a different feature name is fine. Before landing though we'll want to open an issue at rust-lang/rust to track this and fill in the issue number though

[gnzlbg]

I've opened a tracking issue rust-lang/rust#59414

@mehcode can you update the issue number to 59414 ?