8
\$\begingroup\$

My goal with this bit of code is to check if my processor supports rdrand and, if not, execute some other random number generating function. To check if rdrand is supported, the 30th bit in the ecx register should be set.

I guess my one of my dilemmas is whether or not I should explicitly check inside level 1 but I think eax implicitly sets the level there. Additionally, I wonder if I should set initialize the values of the registers to 0. I'd also like to know of any 'gotchas' and possible improvements.

#include <stdio.h>
#include <string.h>

#define bit_RDRND   (1 << 30)

int main(int argc, char **argv)
{

    unsigned int eax;
    unsigned int ebx;
    unsigned int ecx;
    unsigned int edx;

    eax = 0x01;

    __asm__ __volatile__(
                         "cpuid;"
                         : "=a"(eax), "=b"(ebx), "=c"(ecx), "=d"(edx)
                         : "a"(eax)
                         );

    printf("The value of the ecx register is %08x.\n", ecx);

    if(ecx & bit_RDRND){
        //use rdrand
        printf("use rdrand\n");
    }
    else{
        //use mt19937
        printf("use mt19937");
    }

    return 0;
}
\$\endgroup\$
10
\$\begingroup\$

Let's look at the inline assembly code that you have:

  • You were correct to use the __volatile__ specifier here. That's a common mistake when writing inline asm, so that's a good start. The CPUID instruction has very important side-effects, and you don't want the compiler deciding to elide it in the name of "optimization". (Note: I may be wrong here, and __volatile__ may not actually be necessary for CPUID. The official documentation, as I read it, is rather unclear, and I confess to not fully understanding the nuances. See also discussion in comments below.)
  • You don't need a semicolon after the cpuid instruction in the inline assembly block. For only a single instruction, no terminator is needed. If you have multiple instructions, standard practice is simply to separate them with \n\t so that they will be nicely formatted in the compiler's intermediate assembly output. A semicolon won't affect the correctness of the code, of course, but they might confuse a human reader of the code who isn't expecting to see one there. They also just clutter up the compiler's intermediate asm output, should you ever need to analyze it.
  • This is purely stylistic, but I like to align the colons in the inline asm syntax in columns with the opening and closing parentheses. The way you have it now is perfectly readable, though. This starts to matter more when you have multiple instructions in a single asm block.
  • You also expressed concern in the question about whether you should initialize the values of the registers to 0. In fact, you seem to be slightly confused about exactly how to call CPUID. The authoritative reference is always the Intel manuals, but transcriptions have been made and placed online by multiple folks. Searching Google for the instruction name + x86 makes them very easy to find. Here is the documentation for CPUID. It tells you exactly how it needs to be called.

    The summary is, EAX needs to be set to the "function"/"level", which indicates the type of information to retrieve. In your case, to get the feature bits, EAX should contain 1. The other registers do not need to be pre-initialized; they will be clobbered and filled by the CPU during the execution of CPUID. The code you have is therefore correct as-written; there are no hidden "gotchas" to be aware of (at least, as long as you are using CPUID as shown here; it is a very "overloaded" instruction!).

  • Finally, I know this is probably just demo code (are you expected to post real code here?), but you really should wrap ugliness like this up in a function. Not only because it looks cleaner, but it also helps to consolidate the scope when the need arises to maintain it. Here's how I'd write it (using my preferred naming conventions; use whatever matches your project):

    void InvokeCPUID(unsigned int  function,
                     unsigned int  subfunction,
                     unsigned int* pEAX,
                     unsigned int* pEBX,
                     unsigned int* pECX,
                     unsigned int* pEDX)
    {
        assert(pEAX != NULL);
        assert(pEBX != NULL);
        assert(pECX != NULL);
        assert(pEDX != NULL);
    
        __asm__ __volatile__("cpuid"
                            : "=a" (*pEAX),
                              "=b" (*pEBX),
                              "=c" (*pECX),
                              "=d" (*pEDX)
                            :  "a" (function),
                               "c" (subfunction)
                            );
    }
    
    _Bool SupportsRDRAND()
    {
        const unsigned int flag_RDRAND = (1 << 30);
    
        unsigned int eax, ebx, ecx, edx;
        InvokeCPUID(1, 0, &eax, &ebx, &ecx, &edx);
    
        return ((ecx & flag_RDRAND) == flag_RDRAND);
    }
    

    The object code emitted by GCC and Clang for SupportsRDRAND (with the call to CPUID inlined) is beautifully simple:

    SupportsRDRAND:
        push    ebx        ; preserve caller-save EBX (required by calling convention)
        mov     eax, 1     ; set EAX to request function 1
        xor     ecx, ecx   ; set ECX to request subfunction 0
        cpuid
        shr     ecx, 30    ; the result we want is in ECX...
        and     ecx, 1     ; ...test for the flag of interest...
        mov     eax, ecx   ; ...and put the result in EAX so it can be returned
        pop     ebx        ; restore previously-saved value of EBX
        ret
    

That said…it is my recommendation that you not use inline assembly at all! I know its raw power is quite seductive, and sometimes you truly have no choice, but you really should try to avoid inline assembly if at all possible. Architectural portability obviously isn't a compelling argument here, but the fact that inline assembly disrupts the optimizer is still important, and even more important is that it vastly increases the development time and maintenance costs. You really do need to be an expert to get the inline assembly code right. And even if you are such an expert (and we still get it wrong regularly), the maintenance programmer who comes along later is invariably not.

Intrinsics are virtually always a better choice, unless you can really demonstrably outsmart the compiler—and even then, only if the code is a real performance bottleneck to justify the maintenance cost. In this case, it is absolutely not necessary. There are intrinsics/built-ins for the CPUID instruction, so you don't need to resort to inline assembly.

Based on the inline assembly syntax used here, you're obviously targeting a Gnu-style compiler. GCC and Clang both support the __get_cpuid built-in, but you'll need to include the <cpuid.h> header to use it. (If you were using MSVC, it has an equivalent __cpuid built-in, in <intrin.h>, that you could use.) Here's how I'd write the code:

#include <cpuid.h>   // for __get_cpuid intrinsic
_Bool SupportsRDRAND()
{
    const unsigned int flag_RDRAND = (1 << 30);

    unsigned int eax, ebx, ecx, edx;
    __get_cpuid(1, &eax, &ebx, &ecx, &edx);

    return ((ecx & flag_RDRAND) == flag_RDRAND);
}

Clang does an outstanding job with this code, producing object code virtually identical to that shown above. On the other hand, seemingly intent on making a liar out of me regarding the virtues of intrinsics, GCC makes an absolute mess of it. The reason for the "mess" is that it is calling CPUID twice: the first time with EAX set to 0 in order to obtain the highest function/level that the CPU supports, and then the second time with EAX set to the requested function/level. It appears to be doing this out of an abundance of caution, and perhaps based on a literal reading of the Intel documentation, which does recommend this. However, as far as I'm concerned, it is completely unnecessary. All CPUs that support the CPUID instruction support at least function/level 1, so the compiler should be able to detect that this is what you've requested and elide the first verificatory call to CPUID. GCC is clearly not making any attempt whatsoever to optimize the translation of the __get_cpuid intrinsic, since even calling it with the function/level set to 0 results in two invocations of CPUID, this time clearly redundant. While this isn't a bug per se, it is a pretty significant missed optimization opportunity.

The good news, though, is that a call to CPUID is never on the critical path for an application, so even the slowest calling code in the world cannot be a bottleneck. This is probably the counter-argument to be made for GCC's failure to optimize __get_cpuid, and means it is not worth losing sleep over. If for some reason you did find yourself losing sleep over this, you could always resort to using inline assembly, as discussed above. Or, you could use the internal __cpuid macro (which is used internally by the __get_cpuid intrinsic):

#include <cpuid.h>     // for __cpuid
_Bool SupportsRDRAND()
{
    const unsigned int flag_RDRAND = (1 << 30);

    unsigned int eax, ebx, ecx, edx;
    __cpuid(1, eax, ebx, ecx, edx);

    return ((ecx & flag_RDRAND) == flag_RDRAND);
}

This brings the object code emitted by GCC into line with my expectations, like that shown above. (Which makes sense, considering the definition of the macro is essentially identical to our inline asm above!) Although I can't find evidence from the Gnu folks that this is documented and intended for public consumption, it does work on GCC, Clang, and ICC. (This is also the name of the corresponding intrinsic for MSVC, although it is called a bit differently.)


Other Notes:

  • The above discussion of code quality reminds me of an important point that needs to be pointed out explicitly: when using CPUID, you should call it at application initialization to obtain the desired information, and then immediately cache those results away in a global variable. Whenever you want to make dynamic-dispatching decisions during the course of your application's execution, check the global flag—don't re-call CPUID each time! There is no need to pay the speed penalty (even with the most optimal calling code generation, CPUID itself is an extremely slow instruction), because the CPU is not going to change during your application's lifetime.

  • Notice that I have written my bit-testing code slightly differently than yours. You are just ANDing the mask with the bits, and then testing if the result is non-zero:

    if (ecx & flag_RDRAND) { ... }
    

    By contrast, I prefer to explicitly test that the result is equal to the mask:

    ((ecx & flag_RDRAND) == flag_RDRAND) { ... }
    

    In the majority of cases, the results will be identical, but I prefer this for readability and generality. In the case that you have a multi-bit mask, comparing against zero may lead to an incorrect result, as it will evaluate to non-zero if either of the flags are set. Also, in the event that you ever have a mask of 0 (which you shouldn't, of course), writing the comparison the way I've done will ensure that you get the expected result.

    The way I look at it is, writing code is difficult enough already, so the fewer things you have to remember and think about, the better. If you can write bit-testing code that is always correct without ever having to think about it, then I believe that's what you should do.

  • If possible, prefer not using macros to define constants! I realize there are cases in C where you actually do need to use a #define (or possibly an enum), and a static const variable simply won't work since it's not a true constant (e.g., when declaring the size of a memory array in a structure), but I believe you should prefer const objects whenever possible. These are more debugger-friendly, avoid namespace pollution, respect scoping rules, and are type-safe. Any decent optimizing compiler should fold constants, so the resulting binary will be equivalent. If you're on an embedded system, the rules may be different, but this code is clearly x86-specific!

  • Dair already noted your use of printf as opposed to puts. I guess he's probably right; I can't argue with the statement that simpler is usually better than more complicated. But this is obviously just demo code, not a demonstration of what you're actually going to be doing with these results. Furthermore, which function you use for console output is just not something that matters in real-world code. Both functions are so slow from an optimization perspective that the potentially-minor speed improvement of puts over printf is unimportant. The bottleneck there is not the speed of the output, but the speed at which a human operator can perceive the stimuli. Besides, a sufficiently smart compiler may optimize printf into puts when possible.


Next Steps:

Finally, harking back to the discussion at the top of why you should always prefer intrinsics to inline assembly, when you get ready to invoke RDRAND on the processors that you determine have support for it, make sure that you use an intrinsic to do so.

The following are the intrinsics for RDRAND, parameterized on the size (in bits) of the random integer to be generated:

  • int _rdrand16_step(unsigned short*)
  • int _rdrand32_step(unsigned int*)
  • int _rdrand64_step(unsigned long long*)

These write the generated random value into to the specified pointer, and return an int indicating success (1) or failure (0). To use them, you must include the <x86intrin.h> header (or, specifically, <immintrin.h>).

Usage is drop-dead simple. If you wanted to retain the ability to check for success, you could do something like:

#include <x86intrin.h>   // for _rdrand32_step intrinsic
_Bool GetRandomViaRDRAND(unsigned int* pResult)
{
    // This function should *only* be called if the underlying processor supports
    // the RDRAND instruction. This is the caller's responsibility to verify,
    // e.g., using the CPUID instruction. For performance reasons,
    // we don't want to verify this each time a random number is desired,
    // but to ensure correctness, we will validate it in debugging builds.
    assert(SupportsRDRAND());

    return (_rdrand32_step(pResult) != 0);
}

Or, if you want to play it dangerously and ignore the hardware's success flag, you could do the following:

unsigned int GetRandomViaRDRAND()
{
    // This function should *only* be called if the underlying processor supports
    // the RDRAND instruction. This is the caller's responsibility to verify,
    // e.g., using the CPUID instruction. For performance reasons,
    // we don't want to verify this each time a random number is desired,
    // but to ensure correctness, we will validate it in debugging builds.
    assert(SupportsRDRAND());

    unsigned int value;
    int result;
    result = _rdrand32_step(&value);
    assert(result != 0);
    return value;
}

This not only simplifies the interface by which the function is called, but also results in slightly more efficient object code. Because you're using an intrinsic, the compiler can tell whether or not you're actually using its return value. If you do, it has to store the carry flag (CF is set by the CPU after executing RDRAND) in such a way that you can test it (compilers use a conditional move instruction for this). If you don't use the return value, it can elide this code altogether, executing only an RDRAND instruction.

If you did need to fall back to inline assembly for RDRAND, hopefully you know how to do it correctly now. Notice that we need to use the cc clobber, since the instruction clobbers the flags:

unsigned int GetRandomViaRDRAND()
{
    unsigned int value;
    __asm__("rdrand  %[value]"
           : [value] "=r" (value)
           :       /* no inputs */
           : "cc"  /* clobbers flags (condition codes) */
           );
    return value;
}
\$\endgroup\$
  • \$\begingroup\$ volatile qualifier: Are you sure? If you were to wrap OP's code in for(x=0;x<10;x++), would the optimizer be correct in moving the asm out of the loop? I say yes, which means no volatile qualifier. For some assemblers (like gas), semicolons are statement delimiters and '#' marks comments. The reason gcc calls cpuid twice is to validate the 'level' of cpuid you are calling (see __get_cpuid in cpuid.h). Also, compilers can replace printf with puts during optimization. \$\endgroup\$ – David Wohlferd Dec 18 '16 at 23:37
  • \$\begingroup\$ @david Thanks for your response. Regarding volatile, no I'm not sure. I confess to not completely understanding its effect. All I can go on is what I've read in the docs and various places online. The docs suggest that volatile indicates an instruction has important side effects and therefore cannot be elided. I'd say CPUID qualifies for that, so to be safe, I'd make it volatile. Maybe it isn't necessary here, since all the outputs are explicit? As far as reordering, the docs say that "even a volatile asm instruction can be moved relative to other code, including across jump instructions". \$\endgroup\$ – Cody Gray Dec 20 '16 at 10:48
  • \$\begingroup\$ I could be wrong and volatile is unnecessary, but I'd rather be on the safe side here, since this is not in any way perf critical code. Also, yes, you're right about semicolons/octothorpes. My native dialect is Intel/MASM, so GAS is always like a foreign language to me. I'll update that part of the answer. I'm also planning an update regarding GCC calling CPUID twice. I discovered that but didn't get a chance to update. Looks like a bug to me, though. Function 1 is always available on any processor that supports CPUID, so GCC should be able to see you're passing 1 and elide level validation. \$\endgroup\$ – Cody Gray Dec 20 '16 at 10:51
  • \$\begingroup\$ As evidence for the volatile qualifier being unnecessary, I point to __get_cpuid in cpuid.h, which has no volatile qualifier. You are correct that always adding 'volatile' is safer, but this seems to result in people putting it on every single asm instruction, "just to be safe." This inhibits the few optimizations gcc might be able to apply to the asm. When I think of volatile asm, I think in terms of "changing the floating point rounding mode" or "inputting a byte from a hardware port." Even if the actual outputs of the asm are unused, you can't omit these asms, else subsequent code fails. \$\endgroup\$ – David Wohlferd Dec 20 '16 at 23:27
  • 1
    \$\begingroup\$ @DavidWohlferd Personally I mark it as volatile because a potential side effect someone may rely on is that CPUID does serialization. Last thing I want is for an optimizer to potentially elide it or hoist it out of a loop. \$\endgroup\$ – Michael Petch Sep 5 '18 at 20:21
3
\$\begingroup\$

This:

printf("The value of the ecx register is %08x.\n", ecx);

is fine. However for the other two:

    printf("use rdrand\n");

and:

    printf("use mt19937");

Simpler is better usually better than more complicated, I would recommend you use puts instead. So:

    puts("use rdrand");

and:

    puts("use mt19937");

Furthermore, puts automatically adds a newline automatically so you don't have to worry about it. (Also, I am assuming you want a \n after "use mt19937" since you have one in "use rdrand\n".)

\$\endgroup\$
  • \$\begingroup\$ I can't disagree with the point you're making here, I but feel like this answer fundamentally misses the point of the question, which is to review his use of inline assembly to execute the CPUID instruction (to dynamically check support for another instruction). It seems pretty obvious to me that the code in the question is just demo code. The printf calls are just placeholders; the plan is to actually use the appropriate functions once ascertaining support. Although I'm new here, I imagine the expectation is that you post real code, so I can see how you may have been misled. \$\endgroup\$ – Cody Gray Dec 18 '16 at 19:54
  • \$\begingroup\$ @Cody Gray: Yes, it looks like a placeholder, but the mentality I've taken toward this site is the code isn't perfect unless it is perfect. It may be that this code correction is "pointless" for this project, but now he knows about this and can apply it for later projects. But for sure your answer is more in depth, I will up vote it. \$\endgroup\$ – Dair Dec 18 '16 at 20:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.