x86_cpuid_halide must preserve all 64 bits of rbx/rsi #6409
Conversation
The existing code attempts to preserve ebx (since the cpuid instruction can trash it), but it only preserves the lower 32 bits; on 64-bit systems, this (amazingly) usually works OK unless you are compiling in (e.g.) ASAN mode, which can subtly change codegen such that the full 32 bits of rbx must be preserved. I'm genuinely astonished this hasn't bitten us before now!
|
I think it hasn't bitten us because the inline assembly marks ebx as clobbered, so there's no actual need to restore it. For the purposes of register allocation, that causes llvm to push/pop all of rbx in the containing function (because it's callee-saved), and then not otherwise use rbx. So the effect of marking ebx as clobbered is, most of the time, to get llvm to not use rbx in that function, and to save and restore it at the containing function entry/exit. Asan mode must be doing something different that makes the compiler actually care about the difference between clobbering ebx and clobbering rbx. |
|
So +1 to the change, but I'm confused about why we explicitly save and restore e/rbx in the inline assembly as well as marking it as a clobber. |
abadams
added
the
backport me
The existing code attempts to preserve ebx (since the cpuid instruction can trash it), but it only preserves the lower 32 bits; on 64-bit systems, this (amazingly) usually works OK unless you are compiling in (e.g.) ASAN mode, which can subtly change codegen such that the full 32 bits of rbx must be preserved. I'm genuinely astonished this hasn't bitten us before now! (cherry picked from commit 02a394d)
The existing code attempts to preserve ebx (since the cpuid instruction can trash it), but it only preserves the lower 32 bits; on 64-bit systems, this (amazingly) usually works OK unless you are compiling in (e.g.) ASAN mode, which can subtly change codegen such that the full 32 bits of rbx must be preserved. I'm genuinely astonished this hasn't bitten us before now! (cherry picked from commit 02a394d)
alexreinking
added
bug
and removed
backport me
The existing code attempts to preserve ebx (since the cpuid instruction can trash it), but it only preserves the lower 32 bits; on 64-bit systems, this (amazingly) usually works OK unless you are compiling in (e.g.) ASAN mode, which can subtly change codegen such that the full 32 bits of rbx must be preserved.
I'm genuinely astonished this hasn't bitten us before now!