Re: [Valgrind-developers] tests/x86_amd64_features.c

[Florian K. <fl...@ei...>]

On 23.07.25 22:29, Mark Wielaard wrote:
> Hi Florian,
> 
> On Wed, Jul 23, 2025 at 11:53:54AM +0200, Florian Krohm wrote:
>> While regtesting a patch for x86 to remove Iop_Clz32 (BZ 507033) I
>> noticed that the testcase none/tests/x86/lzcnt32 was not executed
>> due to lack of required prereqs. This of course is *the* interesting
>> testcase here.
>>
>> The reason it's not executed is because tests/x86_amd64_features.c has this:
>>
>>     if (require_amd && !vendorStringEquals("AuthenticAMD"))
>>        return FEATURE_NOT_PRESENT;
>>
>> My laptop identifies itself as "GenuineIntel". Therefore I propose
>> the regtested patch below. Anybody sees a problem with that?
> 
> I think the issue is that LZCNT really is (or was?) AMD only. If
> executed on Intel it behaves like BSR (which is encoded the same). So
> the check as written seems as intended.
> 
> But I believe since that test was written Intel introduced BMI, which
> includes LZCNT.
> 
> So it might be that the x86-lzcnt (and amd64-lzcnt?) checks need to be
> rewritten to detect the cpu supports them (and executes them as LZCNT,
> not as BSR). But I am don't think just adding
> !vendorStringEquals("GenuineIntel") is the correct way to do that.
> 

As discussed on IRC here's a revised version. Regtested. OK?

diff --git a/tests/x86_amd64_features.c b/tests/x86_amd64_features.c
index 5a95e06df..7b58c1e92 100644
--- a/tests/x86_amd64_features.c
+++ b/tests/x86_amd64_features.c
@@ -102,7 +102,6 @@ static Bool go(char* cpu)
     } else if ( strcmp( cpu, "x86-lzcnt" ) == 0 ) {
       level = 0x80000001;
       cmask = 1 << 5;
-     require_amd = True;
  #if defined(VGA_amd64)
     } else if ( strcmp( cpu, "amd64-sse3" ) == 0 ) {
       level = 1;
@@ -119,7 +118,6 @@ static Bool go(char* cpu)
     } else if ( strcmp( cpu, "amd64-lzcnt" ) == 0 ) {
       level = 0x80000001;
       cmask = 1 << 5;
-     require_amd = True;
     } else if ( strcmp( cpu, "amd64-sse42" ) == 0 ) {
       level = 1;
       cmask = 1 << 20;
@@ -130,7 +128,6 @@ static Bool go(char* cpu)
     } else if (strcmp (cpu,  "amd64-fma4" ) == 0) {
       level = 0x80000001;
       cmask = 1 << 16;
-     require_amd = True;
     } else if (strcmp (cpu,  "amd64-f16c" ) == 0) {
        level = 1;
        cmask = 1 << 29;
@@ -148,8 +145,7 @@ static Bool go(char* cpu)
     assert( !(cmask != 0 && dmask != 0 && bmask != 0) );
     assert( !(cmask == 0 && dmask == 0 && bmask == 0) );

-   if (require_amd && !vendorStringEquals("AuthenticAMD") &&
-       !vendorStringEquals("GenuineIntel"))
+   if (require_amd && !vendorStringEquals("AuthenticAMD"))
        return FEATURE_NOT_PRESENT;
        // regardless of what that feature actually is