Avoid using ebx as an asm! operand #185
Conversation
rust LLVM minimum version is now 10 This reverts commit 349183e.
failes compilation as it is sometimes reserved by LLVM.
makes it more clear what clobbers registers also results in one redundant move is removed for rdpmc_pair
|
r? @eddyb |
There was a problem hiding this comment.
Had to review one commit at a time, but this makes sense. Sorry I haven't gone back to this yet. I ran into the ebx thing while trying to get the standalone "SpecLockMap checker" compiling on recent nightlies, for someone, and this looks like the correct fix.
There's only one concern I have (about the cpuid(0) code size with the ebx workaround), for which I left comments.
measureme/src/counters.rs
Outdated
| // LLVM sometimes reserves `ebx` for its internal use, we so we need to use | ||
| // a scratch register for it instead. | ||
| "mov {tmp_rbx:r}, rbx", | ||
| "cpuid", | ||
| "mov rbx, {tmp_rbx:r}", |
There was a problem hiding this comment.
I've noticed this. This is frustrating, as it makes cpuid undesirably troublesome (I wish Intel and AMD would agree on e.g. mfence/lfence being serializing). I suspect that using a LLVM intrinsic for cpuid would result in shorter codegen, can you try this? Either way, can you PM me on Zulip?
I've let that rustc PR sit, sadly (long story short, other factors forced us to abandon the grant it was part of), and it would be good to take stock of what's useful and what's just hacks we only needed to get the results we wanted.
| asm!( | ||
| "rdpmc", | ||
| in("ecx") reg_idx, | ||
| lateout("eax") lo, | ||
| lateout("edx") hi, | ||
| options(nostack) |
There was a problem hiding this comment.
I suppose this is good because it gives LLVM just one instruction and all of the register-wiring insight it needs.
| /// Dummy `cpuid(0)` to serialize instruction execution. | ||
| #[inline(always)] | ||
| fn serialize_instruction_execution() { |
There was a problem hiding this comment.
Should be able to take this fn in isolation and try it on godbolt, for the comparison with std::arch::x86_64::__cpuid(0);.
There was a problem hiding this comment.
minor difference with using arch version one extra xor and xchg instead of the mov that I used.
when I tried to rebase rust-lang/rust#78781 I noticed that due to rust-lang/rust#84658 this code did not compile any longer.
also due to rust-lang/rust#83387 is merged we can use the intel asm and remove the duplication of all asm instructions