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.

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
merged 11 commits into from
Jun 25, 2018
76 changes: 46 additions & 30 deletions coresimd/x86/cpuid.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,26 +86,51 @@ 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 21st bit of the EFLAGS register is modifiable or not.
// If it is, then `cpuid` is available.
let eax: i32;
asm!(r#"
# Save ecx (__fastcall needs it preserved) and save a

Choose a reason for hiding this comment

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

I would expect either saving ecx in the asm or declaring it as clobbered, not both. If it's in the list of clobbered registers, the backend is responsible for saving and restoring ecx if it contains a live value at the locations. Since that's simpler and less error prone than manual saving and restoring (and can even avoid the save-restore dance altogether in many cases), I'd prefer that.

# copy of the original eflags that we will restore later:
push %ecx
pushfd
# Copy eflags to ecx and eax:
pushfd
pop %eax
mov %ecx, %eax
# Flip 21st bit and write back to eflags register:
xor %eax, 0x200000
push %eax
popfd
# Read eflags register again:
pushfd
pop %eax
# If cpuid is available, the bit will still be flipped
# and it will be the only bit modified.
#
# xor with the original eflags should produce a 1 in
# the 21st bit in this case, and zeros for all other bits:
xor %eax, %ecx
# So if the value of the 21st bit is 1, cpuid is available,
# and if it is zero, it isn't because we didn't manage to
# modify it:
shrd %eax, 21
Copy link
Contributor Author

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.

# Restore original eflags and ecx:
popfd
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.

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

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

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.

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,17 +163,8 @@ mod tests {
assert!(cpuid::has_cpuid());
}

#[cfg(target_arch = "x86")]
#[test]
fn test_has_cpuid() {
unsafe {
let before = __readeflags();

if cpuid::has_cpuid() {
assert!(before != __readeflags());
} else {
assert!(before == __readeflags());
}
}
fn test_has_cpuid_idempotent() {
assert_eq!(cpuid::has_cpuid(), cpuid::has_cpuid());
}
}