fix: Add `x86_64` archspec support for Windows #3803

jjerphan · 2025-02-05T12:09:30Z

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

libmamba/src/core/virtual_packages.cpp

intentionally-left-nil · 2025-02-11T18:21:47Z

Hi Mamba folks, is there any particular testing that would be helpful to get this PR over the line? Or is it just waiting for someone to review it

jjerphan · 2025-02-11T18:24:06Z

@intentionally-left-nil: we need someone to test and review it, ideally on several Windows machines with different architectures.

We welcome your feedback greatly! 🙂

JohanMabille

LGTM although having feedback from people who can test it would be nice.

The nitpicking comments can be ignored if you prefer to keep the exhaustive check list for each version of x86_64.

libmamba/src/core/virtual_packages.cpp

JohanMabille · 2025-02-13T10:04:27Z

libmamba/src/core/virtual_packages.cpp

+            bool avx512cd = cpu_info[1] & (1 << 28);
+            bool avx512bw = cpu_info[1] & (1 << 30);
+            bool avx512vl = cpu_info[1] & (1 << 31);
+            if (avx512f && avx512dq && avx512cd && avx512bw && avx512vl)


Nitpicking: avx512 dq/cd/bw/vl are all implemented on top of avx512f, so no need to check for this value.

I agree, but I propose to keep the logic in line with the one for unix system as currently implemented a few lines above because it is cheap and it does not make assumption (also, I would take time to make sure that the above is correctly implemented, because more instructions seem to be present for each architecture).

JohanMabille · 2025-02-13T10:04:54Z

libmamba/src/core/virtual_packages.cpp

+            bool bmi2 = cpu_info[1] & (1 << 8);
+            bool fma_ = cpu_info[1] & (1 << 12);
+            bool avx = cpu_info[1] & (1 << 28);
+            if (bmi && avx2 && bmi2 && fma_ && avx)


Similar remark here: avx2 is implmeented on top of avx, so no need to check for avx.

JohanMabille · 2025-02-13T10:11:22Z

libmamba/src/core/virtual_packages.cpp

+            bool sse41 = cpu_info[1] & (1 << 19);
+            bool sse42 = cpu_info[1] & (1 << 20);
+            bool popcnt = cpu_info[1] & (1 << 23);
+            if (sse3 && ssse3 && sse41 && sse42 && popcnt)


Similar remark here: sse42 implies sse41 which implies ssse3 which implies sse3,so checking sse42 should be enough (I haven't checked for popcnt).

Same remark here, the support for all these instructions should be read from cpu_info[2] after call to cpuid with eax=1.

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz> Co-authored-by: Johan Mabille <johan.mabille@gmail.com>

Klaim · 2025-02-13T16:57:01Z

I tested on my Windows machine, it has an AMD Ryzen 9 5900X , running Windows 11.

I built this branch locally using Visual Studio (preview) v17.13.0-pre.2.1, compiler version 19.43.34618 for x64.
I obtained:

mamba info -v

       libmamba version : 2.0.6
          mamba version : 2.0.6
           curl version : libcurl/8.11.1 Schannel zlib/1.3.1 libssh2/1.11.1
     libarchive version : libarchive 3.7.7 zlib/1.3.1 liblzma/5.6.3 bz2lib/1.0.8 liblz4/1.10.0 libzstd/1.5.6
       envs directories : C:\Users\Klaim\micromamba\envs
          package cache : C:\Users\Klaim\micromamba\pkgs
                          C:\Users\Klaim\.mamba\pkgs
                          C:\Users\Klaim\AppData\Roaming\.mamba\pkgs
            environment : mamba-pr-testing (active)
           env location : C:\Users\Klaim\micromamba\envs\mamba-pr-testing
      user config files : C:\Users\Klaim\.mambarc
 populated config files :
       virtual packages : __win=10.0.22631=0
                          __archspec=1=x86_64_v2
                          __cuda=12.7=0
               channels : https://conda.anaconda.org/conda-forge/noarch
                          https://conda.anaconda.org/conda-forge/win-64
       base environment : C:\Users\Klaim\micromamba
               platform : win-64

I was told __archspec should be v3 for this case? (I have no idea what is the correct value for this cpu)

jjerphan · 2025-02-13T17:06:24Z

AMD Ryzen 9 5900X qualifies for x86_64_v3.

nbeerbower · 2025-02-13T22:07:57Z

Perhaps I compiled something wrong but my results on Windows 11 with an i9-13950HX:


       libmamba version : 2.0.6.rc2
          mamba version : 2.0.6.rc2
           curl version : libcurl/8.11.1 Schannel zlib/1.3.1 libssh2/1.11.1
     libarchive version : libarchive 3.7.7 zlib/1.3.1 liblzma/5.6.3 bz2lib/1.0.8 liblz4/1.10.0 libzstd/1.5.6
       envs directories : C:\Users\Nick\.local\share\mamba\envs
          package cache : C:\Users\Nick\.local\share\mamba\pkgs
                          C:\Users\Nick\.mamba\pkgs
                          C:\Users\Nick\AppData\Roaming\.mamba\pkgs
            environment : build_env (active)
           env location : C:\Users\Nick\.local\share\mamba\envs\build_env
      user config files : C:\Users\Nick\.mambarc
 populated config files :
       virtual packages : __win=10.0.26100=0
                          __archspec=1=x86_64
                          __cuda=12.4=0
               channels : https://conda.anaconda.org/conda-forge/noarch
                          https://conda.anaconda.org/conda-forge/win-64
       base environment : C:\Users\Nick\.local\share\mamba
               platform : win-64

Compiler: Microsoft (R) C/C++ Optimizing Compiler Version 19.29.30154 for x64

JohanMabille · 2025-02-14T07:50:09Z

@nbeerbower I think you're testing the wrong binary, the output version is 2.0.6.rc2, while it should be 2.0.6.

JohanMabille · 2025-02-14T08:26:20Z

libmamba/src/core/virtual_packages.cpp

+            if (cpu_info[0] < 7)
+            {
+                return "x86_64";
+            }


I'm not sure of this, eax>7 is required to get supported instructions for v4, but instructions for lower versions (such as v3) are checked with a lower value of eax (see my other comment below).

JohanMabille · 2025-02-14T08:27:53Z

libmamba/src/core/virtual_packages.cpp

+            bool avx2 = cpu_info[1] & (1 << 5);
+            bool bmi2 = cpu_info[1] & (1 << 8);
+            bool fma_ = cpu_info[1] & (1 << 12);
+            bool avx = cpu_info[1] & (1 << 28);


The support for fma and avx should be checked after a call to cpuid with eax = 1, and read from ecx (i.e. cpu_info[2]), see https://en.wikipedia.org/wiki/CPUID#EAX=1:_Processor_Info_and_Feature_Bits

Sorry for not catching it during the first review.

JohanMabille · 2025-02-14T08:30:39Z

libmamba/src/core/virtual_packages.cpp

+            bool sse41 = cpu_info[1] & (1 << 19);
+            bool sse42 = cpu_info[1] & (1 << 20);
+            bool popcnt = cpu_info[1] & (1 << 23);
+            if (sse3 && ssse3 && sse41 && sse42 && popcnt)


Same remark here, the support for all these instructions should be read from cpu_info[2] after call to cpuid with eax=1.

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz> Co-authored-by: Johan Mabille <johan.mabille@gmail.com>

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

nbeerbower · 2025-02-17T20:06:59Z

My bad! I cloned the fork repo but didn't checkout the PR branch.

       libmamba version : 2.0.6
          mamba version : 2.0.6
           curl version : libcurl/8.11.1 Schannel zlib/1.3.1 libssh2/1.11.1
     libarchive version : libarchive 3.7.7 zlib/1.3.1 liblzma/5.6.3 bz2lib/1.0.8 liblz4/1.10.0 libzstd/1.5.6
       envs directories : C:\Users\Nick\.local\share\mamba\envs
          package cache : C:\Users\Nick\.local\share\mamba\pkgs
                          C:\Users\Nick\.mamba\pkgs
                          C:\Users\Nick\AppData\Roaming\.mamba\pkgs
            environment : build_env (active)
           env location : C:\Users\Nick\.local\share\mamba\envs\build_env
      user config files : C:\Users\Nick\.mambarc
 populated config files :
       virtual packages : __win=10.0.26100=0
                          __archspec=1=x86_64_v3
                          __cuda=12.4=0
               channels : https://conda.anaconda.org/conda-forge/noarch
                          https://conda.anaconda.org/conda-forge/win-64
       base environment : C:\Users\Nick\.local\share\mamba
               platform : win-64

Looks like it works perfectly now! Let me know if you'd like me to test anything else.

jjerphan · 2025-02-18T10:15:37Z

Thank you for testing, @nbeerbower.

I would at least wait for another review from Joël who also uses Windows (it might take some time because this PR is not the most urgent one regarding work on Windows).

intentionally-left-nil · 2025-02-18T16:01:10Z

@jjerphan thanks for the updates. If the main reason is just for extra hardware coverage, is there any scenario in particular? We can try to dig up more machines that match the desired test case.

Otherwise if your main goal is to have more PR reviews, then we'll just patiently wait :)

jjerphan · 2025-02-18T16:10:31Z

Is there any scenario in particular?

Ideally all the four x84_64 versions must be tested.

Klaim · 2025-02-19T15:31:41Z

Here is my result with the current version:

$ mamba info -v

       libmamba version : 2.0.6
          mamba version : 2.0.6
           curl version : libcurl/8.11.1 Schannel zlib/1.3.1 libssh2/1.11.1
     libarchive version : libarchive 3.7.7 zlib/1.3.1 liblzma/5.6.3 bz2lib/1.0.8 liblz4/1.10.0 libzstd/1.5.6
       envs directories : C:\Users\Klaim\micromamba\envs
          package cache : C:\Users\Klaim\micromamba\pkgs
                          C:\Users\Klaim\.mamba\pkgs
                          C:\Users\Klaim\AppData\Roaming\.mamba\pkgs
            environment : mamba-pr-testing (active)
           env location : C:\Users\Klaim\micromamba\envs\mamba-pr-testing
      user config files : C:\Users\Klaim\.mambarc
 populated config files :
       virtual packages : __win=10.0.22631=0
                          __archspec=1=x86_64_v3
                          __cuda=12.7=0
               channels : https://conda.anaconda.org/conda-forge/noarch
                          https://conda.anaconda.org/conda-forge/win-64
       base environment : C:\Users\Klaim\micromamba
               platform : win-64

If my understanding is correct this is the expected output? You'll have to confirm as I dont know much on the cpu capability flags subjects.

jjerphan · 2025-02-24T10:37:11Z

If my understanding is correct this is the expected output? You'll have to confirm as I dont know much on the cpu capability flags subjects.

Yes, it is correct.

jjerphan added the release::bug_fixes label Feb 5, 2025

jjerphan requested a review from Klaim February 5, 2025 12:37

jjerphan marked this pull request as ready for review February 5, 2025 12:37

jjerphan commented Feb 5, 2025

View reviewed changes

libmamba/src/core/virtual_packages.cpp Show resolved Hide resolved

JohanMabille approved these changes Feb 13, 2025

View reviewed changes

JohanMabille reviewed Feb 14, 2025

View reviewed changes

jjerphan and others added 2 commits February 14, 2025 10:17

Be more explicit

Verified

This commit was signed with the committer’s verified signature.

jjerphan Julien Jerphanion

GPG key ID: 56B690A97C2E35B8

Verified
Learn about vigilant mode

Loading
Loading status checks…

3b50db3

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

JohanMabille approved these changes Feb 24, 2025

View reviewed changes

jjerphan merged commit cdcec32 into mamba-org:main Feb 24, 2025
34 checks passed

jjerphan deleted the fix/windows-x86_64-archspec branch February 24, 2025 11:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Add `x86_64` archspec support for Windows #3803

fix: Add `x86_64` archspec support for Windows #3803

jjerphan commented Feb 5, 2025

intentionally-left-nil commented Feb 11, 2025

jjerphan commented Feb 11, 2025

JohanMabille left a comment

JohanMabille Feb 13, 2025

jjerphan Feb 13, 2025

JohanMabille Feb 13, 2025

JohanMabille Feb 13, 2025

JohanMabille Feb 14, 2025

Klaim commented Feb 13, 2025

jjerphan commented Feb 13, 2025

nbeerbower commented Feb 13, 2025

JohanMabille commented Feb 14, 2025

JohanMabille Feb 14, 2025

JohanMabille Feb 14, 2025 •

edited

Loading

JohanMabille Feb 14, 2025

nbeerbower commented Feb 17, 2025

jjerphan commented Feb 18, 2025

intentionally-left-nil commented Feb 18, 2025

jjerphan commented Feb 18, 2025

Klaim commented Feb 19, 2025

jjerphan commented Feb 24, 2025

fix: Add x86_64 archspec support for Windows #3803

fix: Add x86_64 archspec support for Windows #3803

Conversation

jjerphan commented Feb 5, 2025

intentionally-left-nil commented Feb 11, 2025

jjerphan commented Feb 11, 2025

JohanMabille left a comment

Choose a reason for hiding this comment

JohanMabille Feb 13, 2025

Choose a reason for hiding this comment

jjerphan Feb 13, 2025

Choose a reason for hiding this comment

JohanMabille Feb 13, 2025

Choose a reason for hiding this comment

JohanMabille Feb 13, 2025

Choose a reason for hiding this comment

JohanMabille Feb 14, 2025

Choose a reason for hiding this comment

Klaim commented Feb 13, 2025

jjerphan commented Feb 13, 2025

nbeerbower commented Feb 13, 2025

JohanMabille commented Feb 14, 2025

JohanMabille Feb 14, 2025

Choose a reason for hiding this comment

JohanMabille Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

JohanMabille Feb 14, 2025

Choose a reason for hiding this comment

nbeerbower commented Feb 17, 2025

jjerphan commented Feb 18, 2025

intentionally-left-nil commented Feb 18, 2025

jjerphan commented Feb 18, 2025

Klaim commented Feb 19, 2025

jjerphan commented Feb 24, 2025

fix: Add `x86_64` archspec support for Windows #3803

fix: Add `x86_64` archspec support for Windows #3803

JohanMabille Feb 14, 2025 •

edited

Loading