Skip to content

Fix has_cpuid implementation #492

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.

Sign up for GitHub

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

Merged
gnzlbg merged 11 commits into from
Jun 25, 2018
Merged

Fix has_cpuid implementation #492

Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7f12738
Fixes has_cpuid implementation
gnzlbg Jun 23, 2018
a374f60
use inline assembly instead of the __{read,write}eflags intrinsics
gnzlbg Jun 23, 2018
f87c949
fix has_cpuid_test
gnzlbg Jun 23, 2018
5760ad3
fix test and add comments
gnzlbg Jun 23, 2018
b12aef6
no need to save/restore ecx in the inline assembly because its alread…
gnzlbg Jun 23, 2018
277a49a
fix the shift
gnzlbg Jun 23, 2018
3605ad3
Use Stephen Checkoway solution
gnzlbg Jun 24, 2018
c70093b
remove unnecessary shift
gnzlbg Jun 24, 2018
6ee5955
remove unnecesary unsafe block
gnzlbg Jun 24, 2018
9f9427e
use and to test the 21st bit
gnzlbg Jun 24, 2018
1831237
hoist the and out of the inline assembly
gnzlbg Jun 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 35 additions & 19 deletions coresimd/x86/cpuid.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,26 +86,37 @@ pub fn has_cpuid() -> bool {
}
#[cfg(target_arch = "x86")]
{
use coresimd::x86::{__readeflags, __writeeflags};

// On `x86` the `cpuid` instruction is not always available.
// This follows the approach indicated in:
// http://wiki.osdev.org/CPUID#Checking_CPUID_availability
unsafe {
// Read EFLAGS:
let eflags: u32 = __readeflags();

// Invert the ID bit in EFLAGS:
let eflags_mod: u32 = eflags | 0x0020_0000;

// Store the modified EFLAGS (ID bit may or may not be inverted)
__writeeflags(eflags_mod);

// Read EFLAGS again:
let eflags_after: u32 = __readeflags();

// Check if the ID bit changed:
eflags_after != eflags
// On `x86` the `cpuid` instruction is not always available.
// This follows the approach indicated in:
// http://wiki.osdev.org/CPUID#Checking_CPUID_availability
// https://software.intel.com/en-us/articles/using-cpuid-to-detect-the-presence-of-sse-41-and-sse-42-instruction-sets/
// which detects whether `cpuid` is available by checking whether the 21th bit of the EFLAGS register is modifiable or not.
// If it is, then `cpuid` is available.
let eax: i32;
asm!(r#"
push %ecx
pushfd
pushfd
pop %eax
mov %ecx, %eax
xor %eax, 0x200000
push %eax
popfd
pushfd
pop %eax
xor %eax, %ecx
shrd %eax, 21
Copy link
Contributor Author

@gnzlbg gnzlbg Jun 23, 2018

Choose a reason for hiding this comment

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

So the problem is that this is the instruction for doubles, it should be shr, or shrl, but none of these appear to work.

popfd
Copy link

@stevecheckoway stevecheckoway Jun 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Either this line (along with the corresponding pushfd) is going to break test_has_cpuid which is testing that eflags has changed or, more likely, that test was already totally broken and should just be deleted. In fact, since the comparison is going to potentially modify eflags, I don't think that test makes any sense at all.

Copy link
Contributor Author

@gnzlbg gnzlbg Jun 23, 2018

Choose a reason for hiding this comment

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

test_has_cpuid just checks that the function returns true, there is a new test that checks that has_cpuid is indempotent. Maybe both tests should also check that the EFLAGS register wasn't modified, what do you think?

Copy link
Contributor Author

@gnzlbg gnzlbg Jun 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

sorry, I think I didn't fully gotten what you meant on the first read. I just changed the test to verify that calling has_cpuid does not modify the EFLAGS since I think it should be idempotent.

That is, it should try to modify the eflags, see if they were modified, and then revert the modification before returning the result.

Copy link

@stevecheckoway stevecheckoway Jun 23, 2018

Choose a reason for hiding this comment

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

    fn test_has_cpuid() {
        unsafe {
            let before = __readeflags();

            if cpuid::has_cpuid() {
                assert!(before != __readeflags());
            } else {
                assert!(before == __readeflags());
            }
        }
    }

is testing that eflags changes if cpuid is supported and doesn't change if it's not. But the second and third __readeflags() happen after performing a comparison and a conditional jump. The comparison (probably a test instruction in this case) is going to set/reset bits in eflags which makes the comparison with before meaningless.

pop %ecx
"#
: "={eax}"(eax) // output operands
: // input operands
: "memory", "ecx" // clobbers all memory
: "volatile" // has side-effects
);
debug_assert!(eax == 0 || eax == 1);
eax == 1
}
}
}
Expand Down Expand Up @@ -138,6 +149,11 @@ mod tests {
assert!(cpuid::has_cpuid());
}

#[test]
fn test_has_cpuid_idempotent() {
assert_eq!(cpuid::has_cpuid(), cpuid::has_cpuid());
}

#[cfg(target_arch = "x86")]
#[test]
fn test_has_cpuid() {
Expand Down