[DRAFT] enable MIOpen on Windows #2272

apwojcik · 2023-07-25T11:51:42Z

PR to enable compilation of MIOpen on Windows. The list of changes compared to Linux:

Added Windows-specific code,
By default, on Windows, the dependencies are downloaded from remote repositories and compiled during the MIOpen build
- one-time operation during the first build - only external dependencies, the internal, such as rocMLIR, CK, etc., are built separately (llvm-project, rocMLIR, and other ROCm required components are treated as a single project on Windows)
- options exist to use system-installed libraries instead of downloading and building them - can be turned on for all or per individual library.
Installation of *.db files moved to ${CMAKE_CURRENT_BINAR_DIR} instead of source directory
- on Windows, multi-config generators run the installation of kernels in parallel for each configuration. This leads to access violations because another process is busy with a DB file, and the file is locked.
- that is an overhead and data redundancy. Still, we cannot control it otherwise on Windows with multi-config generators (such as MSVC multi-config generator or even Ninja run as a multi-config generator).
Added option to let switch building the tests on or off - it is on by default.
Added default paths for global and local kernels cache on Windows.
Added CMake command miopen_generate_export_header() - using WINDOWS_EXPORT_ALL_SYMBOLS is dangerous. It exposes the public exports, not only the API. Furthermore, more importantly, the number of symbols to export is limited to 64K per single DLL on Windows.
- because tests require public exports, and on Windows, we want to export only API from the DLL, the MIOpen target was split into the MIOpenCore static library and the MIOpen dynamic library. Both tests and MIOpen DLL link to MIOpenCore.
The file cmake/googletest.cmake was renamed to cmake/google-test.cmake - the Windows file system is case insensitive. There's an official module in CMake named GoogleTest. That led to configuration failures because CMake took an incorrect module during the import.
sequences have been upgraded to be used with the Standard Library algorithms, functions, and containers.

averinevg · 2023-07-25T13:07:43Z

@atamazov Please review

averinevg

Size of long integer is 4 bytes on 64-bit Windows and 8 bytes on 64-bit Linux. We have a lot of places in the code where long integers are used. You should replace all of them with exact-width integer types from stdint (or size_t in some cases) or somehow force compiler to treat them as 8 bytes integers.

averinevg · 2023-07-25T13:16:37Z

.gitignore

+# JetBrains IDEs
+.idea/
+cmake-build*/
+build*/


These lines should'n be there, because developers use different IDEs and we can't add all of them

My question is because that is precisely the purpose of .gitignore. Those files typically are huge in other projects. IDEs add a lot of pollution to the project tree. Anyway, we will be adding Visual Studio Code and Visual Studio IDE in other PRs.

Added the final version to this PR.

My question is because that is precisely the purpose of .gitignore.

Take a look at Linux's .gitignore. There is nothing associated with IDEs. IDE typically creates only one directory, and this is not a big deal. You can add your favorite IDE in your own .gitignore.

Also developers usually have several build directories for convenience, so adding build directory to .gitignore makes no sense.

where every single developer would have to exclude every single file/folder from their commits manually every time

I agree that this file must help developers. But this does not mean that you need to add paths to folders with private photos, your diploma project files, and other files not related to the project. Each developer should manually add files to a commit, and not automatically add junk and then try to clean it up.

But why would i have to add paths to such things? These should not and probably will never end up in a project folder. But the folders that the different IDEs create do end up in the project folder. Its not like one moves them there manually and by will, the IDE just creates them there. Thats what the .gitignore is for. To get around this and exclude such folders, so the project is not bloated and "dependant" on a single IDE.

You are missing the point i guess. You dont write random paths in the .gitignore. You add paths there, that definitly could or will end up in the project folder, but are not part of the project.

In Jetbrains IDEs cases, these folders ".idea/", "cmake-build*/", "build*/" will definitly end up in your project folder, if you use that IDE. Not matter if you want it or not (afaik you can configure the different IDEs not to do so, but thats a competely other thing and way out of this scope). So its not like you should add them to the .gitignore, you basically have to if you dont want to check for those files/folders everytime you make any changes/commits in your project. These files and folders will 100% guranteed end up in your project folder and you 100% dont want to have them in your public git repo

where every single developer would have to exclude every single file/folder from their commits manually every time

I agree that this file must help developers. But this does not mean that you need to add paths to folders with private photos, your diploma project files, and other files not related to the project. Each developer should manually add files to a commit, and not automatically add junk and then try to clean it up.

I really think you are missing the point of that file, so i´ll try to explain it differently:
Lets imagine a project on github, a very simple hello world project. So you had the following (broken down) folder Structure:

Hello_World

helloworld.cpp

So you have the Hello_World project folder and a simple helloworld.cpp in that folder, nothing more. Everything fine. Now you open up that project in Visual Studion and now your project folder looks like this:

Hello_World

helloworld.cpp

.vs

Randomly a .vs folder appeared. Now you open the same project in a Jetbrains IDE and the folder structure looks like this:

Hello_World

helloworld.cpp

.vs

.idea

And there we got another folder, that has nothing to do with the project, but ended up in your project folder. You now make some changes to the helloworld.cpp file and want to commit, but git finds the .vs and the .idea folder and wants to add that to your commit, you would have to exclude that from your commit. You make further changes and further commits. You will propably at one point get really annyoed by the fact, that you have to exclude those 2 folders manually every single time you make a commit. Now the .gitignore comes in to play. You add those folders to your .gitignore and will never ever have to think about those folders in your git commits. Git will just skip them.

Now think about a bigger project with more than just one developer and they all use different IDEs. You dont want to rely on every developer to be able to always be 100% certain they have excluded those files and folders before they commit. Mistakes can happen and those files/folders will sooner or later end up in your git project.

To prevent this from ever happening, you exclude those files and folders, that you know will be created by the different IDEs in your .gitignore. Now no developer have to check first and be 100% certain, that they have correctly excluded every single file and folder that a commonly used IDE might have added to they project folder.

Please pay more attention to review codes and cmakefiles. I can agree with @Joly0, but I never think to argue about ignore file is useful.

Totally agree with @scarsty. There shouldnt need to be any argue about the ignore file. It is useful, it is a nice addition by @apwojcik and its smart he thought about adding it in this pr.
Please focus on reviewing the real code and bring this pr and project forward. Many are waiting for a final windows release to use their ROCm cards on Windows

@Joly0 I think you should read @scarsty's comment one more time. I would also like to draw your attention to the fact that this particular pull request is a draft, it was an initial draft. It was decided to split all the work into several subsequent pull requests. If you have constructive suggestions for the entire work, you can present them by creating a new issue or pull request.

CMakeLists.txt

averinevg · 2023-07-25T13:24:34Z

CMakeLists.txt

 endif()

 project ( MIOpen C CXX )

 enable_testing()

+find_package(Threads REQUIRED)


Do we need this package for MIOpen library itself?

Yes, it is linked to the MIOpen dynamic library/shared object, MIOpenDriver, and tests.

In that case, why didn't this dependency exist before?

Windows requires that dependency. We build a single MIOpen.dll - that is the requirement in the project. It does not harm Linux.

Then this dependency must be only for windows

In that case, why didn't this dependency exist before?

Actually it existed in Linux before -> ${CMAKE_THREAD_LIBS_INIT}
https://cmake.org/cmake/help/latest/module/FindThreads.html

cmake/Findzstd.cmake

cmake/google-test.cmake

test/verify.hpp

test/tensor_vec.cpp

test/rnn_vanilla_common.hpp

CAHEK7 · 2023-07-25T15:01:42Z

test/perfdb.cpp

@@ -989,16 +994,55 @@ class DbMultiProcessTest : public DbTest
                if(full_set())
                    command += " --all";

+#if defined(WIN32)


It would be nice to keep that code somewhere in one place. I don't insist but looks like it could be a bit better.

CAHEK7 · 2023-07-25T15:05:07Z

test/include_inliner.cpp

+
+    LPSTR lpCmdLn{};
+
+#if !defined(UNICODE)


This check is missed for all the other cases where we use CreateProcess.
That's why I think it would be better to keep that code in a one place and reuse it everywhere.

test/find_2_conv.cpp

atamazov

All the review comments must be properly resolved prior merging.

dmikushin · 2023-07-27T12:42:33Z

Hi @apwojcik , thank you for working on Windows support. Having MIOpen on Windows is an important milestone, which also improves the quality of our software.

I have two general questions about this PR.

Why bother about MSVC compiler, if we have ROCm clang, and ROCm dependency in any case? Getting our code to support only one compiler is highly preferred. Spending resources on MSVC is questionable, especially given that Microsoft will fade it away sooner or later, in favor of their own Clang flavor. Getting Windows build onto ROCm Clang will also resolve many issues currently being debated here in the comments.
This Windows port relies on the release version of rocMLIR, whereas our MIOpen docker containers are usually based on a specific rocMLIR commit.

Both questions lead to the following issue. Given that our customers will intuitively expect the same MIOpen behavior on Windows and Linux, the approach taken by this PR may lead to significant misunderstanding, a lot of extra work and communication. In this regard, @apwojcik do you agree that Windows and Linux builds should be unified further as much as possible to minimize the risks?

apwojcik · 2023-07-27T12:51:09Z

@dmikushin I invited you to today's meeting

CAHEK7 · 2023-07-27T14:03:19Z

May I ask to not to force-push commits? It's hard to track what happened in-between and since the number of changes is a quite huge, it's hard to re-review everything from the scratch.

averinevg · 2023-07-27T14:04:57Z

cmake/Findzstd.cmake

+#
+# MIT License
+#
+# Copyright (c) 2017 Advanced Micro Devices, Inc.


Suggested change

# Copyright (c) 2017 Advanced Micro Devices, Inc.

# Copyright (c) 2023 Advanced Micro Devices, Inc.

averinevg · 2023-07-27T14:05:16Z

cmake/google-test.cmake

+#
+# MIT License
+#
+# Copyright (c) 2017 Advanced Micro Devices, Inc.


Suggested change

# Copyright (c) 2017 Advanced Micro Devices, Inc.

# Copyright (c) 2023 Advanced Micro Devices, Inc.

apwojcik · 2023-07-27T14:24:50Z

May I ask to not to force-push commits? It's hard to track what happened in-between and since the number of changes is a quite huge, it's hard to re-review everything from the scratch.

I am sorry for that. We are trying to pass the compilation on CI and want the branch clean without intermediate commits. Unfortunately, we do not have a local copy of your CI to test the compilation. We will have it soon. As soon as the compilation passes, the fixes for review comments and failing tests will be added incrementally.

johnnynunez · 2023-08-31T08:46:57Z

@apwojcik How is this pull request going? I would love to see next September/October with ROCM 5.7 to see the 7900xtx.

MidnightKittenCat · 2023-09-11T00:11:04Z

Any changes so far? Are we perhaps scheduled for October-November?

scarsty · 2023-09-11T15:53:58Z

mark...

xTye · 2023-09-18T02:04:22Z

Willing to contribute in any way I can.

johnnynunez · 2023-09-26T10:57:33Z

When will it be merged all the PR that has been opened for windows? @apwojcik

Signed-off-by: Artur Wojcik <artur.wojcik@amd.com> rocblas: disable beta features on Windows

scarsty · 2023-09-29T09:01:43Z

As the review is too slow, I have built a MIOpen.dll with the branch of uif2-initial, and it seems work well on 7900xtx. Please see #2375 (comment)

MidnightKittenCat · 2023-09-29T11:31:48Z

As the review is too slow, I have built a MIOpen.dll with the branch of uif2-initial, and it seems work well on 7900xtx. Please see #2375

This is great! what does this mean for the windows port exactly? are we close to a full release?

scarsty · 2023-09-29T15:25:54Z

As the review is too slow, I have built a MIOpen.dll with the branch of uif2-initial, and it seems work well on 7900xtx. Please see #2375

This is great! what does this mean for the windows port exactly? are we close to a full release?

I am not an official. Everyone knows that miopen is one of (maybe we can remove "one of") the most important component to rocm and Windows is the most widely used platform for business and personal use. After the releasing the rocm of Windows Edition, I have paid attention to this for a period of time but the review progress is worrying.

NeedsMoar · 2023-09-30T12:51:12Z

As the review is too slow, I have built a MIOpen.dll with the branch of uif2-initial, and it seems work well on 7900xtx. Please see #2375

This is great! what does this mean for the windows port exactly? are we close to a full release?

I am not an official. Everyone knows that miopen is one of (maybe we can remove "one of") the most important component to rocm and Windows is the most widely used platform for business and personal use. After the releasing the rocm of Windows Edition, I have paid attention to this for a period of time but the review progress is worrying.

Just out of curiosity, has anyone talked to the ProRender people to find out where the MIOpen.dll they're shipping with every render plugin on Windows is coming from? It appears to be complete as far as the API is concerned and exported functions (not something stripped down). I think ProRender uses it run denoising mainly, I'm not sure what else.

Glancing at the Houdini and Blender plugins, version is v2.0.5.0 / ~7MB / built 11/3/2022

scarsty · 2023-09-30T14:33:31Z

As the review is too slow, I have built a MIOpen.dll with the branch of uif2-initial, and it seems work well on 7900xtx. Please see #2375

This is great! what does this mean for the windows port exactly? are we close to a full release?

I am not an official. Everyone knows that miopen is one of (maybe we can remove "one of") the most important component to rocm and Windows is the most widely used platform for business and personal use. After the releasing the rocm of Windows Edition, I have paid attention to this for a period of time but the review progress is worrying.

Just out of curiosity, has anyone talked to the ProRender people to find out where the MIOpen.dll they're shipping with every render plugin on Windows is coming from? It appears to be complete as far as the API is concerned and exported functions (not something stripped down). I think ProRender uses it run denoising mainly, I'm not sure what else.

Glancing at the Houdini and Blender plugins, version is v2.0.5.0 / ~7MB / built 11/3/2022

that miopen.dll is using OpenCL as backend, which is not good for the new version of rocm.

NeedsMoar · 2023-10-01T02:01:00Z

As the review is too slow, I have built a MIOpen.dll with the branch of uif2-initial, and it seems work well on 7900xtx. Please see #2375

This is great! what does this mean for the windows port exactly? are we close to a full release?

I am not an official. Everyone knows that miopen is one of (maybe we can remove "one of") the most important component to rocm and Windows is the most widely used platform for business and personal use. After the releasing the rocm of Windows Edition, I have paid attention to this for a period of time but the review progress is worrying.

Just out of curiosity, has anyone talked to the ProRender people to find out where the MIOpen.dll they're shipping with every render plugin on Windows is coming from? It appears to be complete as far as the API is concerned and exported functions (not something stripped down). I think ProRender uses it run denoising mainly, I'm not sure what else.
Glancing at the Houdini and Blender plugins, version is v2.0.5.0 / ~7MB / built 11/3/2022

that miopen.dll is using OpenCL as backend, which is not good for the new version of rocm.

Thanks for clearing that up. I can picture multiple ways in which OpenCL isn't the best front-end language to describe the basic data types of tensors never mind that it doesn't seem to be worked on with the goal of at least 1M new vendor extensions per year like... I dunno, Vulkan.

I have both a 7900XTX and an RTX4090 in my primary workstation now so if you need tests run on code which dispatches the equivalent ops to both (I thought I read that it was a thing with ROCm & HIP + PyTorch, which is heavily dependent on this AFAIK; it certainly works with the ncnn-vulkan runners / networks but that's a whole other thing) either to compare results, benchmark, or just make sure it doesn't explode when the combination exists after there's a functional build going, feel free to ping me.

JehandadKhan · 2023-10-11T22:06:25Z

@apwojcik Does this PR and the other windows related PRs overlap or are they exclusive?

scarsty · 2023-10-12T17:43:53Z

In the cmake file

    if(WIN32)
        set(MIOPEN_USER_DB_PATH "$HOME\\\\.miopen\\\\db\\\\" CACHE STRING "Default path to user db files")
        set(MIOPEN_CACHE_DIR "$HOME\\\\.miopen\\\\cache\\\\" CACHE STRING "")
	set(MIOPEN_SYSTEM_DB_PATH "C:\\\\ProgramData\\\\MIOpen\\\\" CACHE PATH "Default path to system db files")
    else()
        set(MIOPEN_USER_DB_PATH "~/.config/miopen/" CACHE STRING "Default path of user db files")
        set(MIOPEN_CACHE_DIR "~/.cache/miopen/" CACHE STRING "")
    endif()

here $HOME is undefined on Windows usually, so the PATH becomes like \\.miopen\\db, which is treated as a net path on Windows. Then the result will be a temp path (src/db_path.cpp.in, about line 98). I think this should be repaired.

averinevg · 2023-10-13T11:49:26Z

In the cmake file

@scarsty Please take a look at #2365

averinevg · 2023-10-13T11:58:24Z

@apwojcik Since all the work has been continued in subsequent PRs, please close this PR if you no longer need it.

apwojcik · 2023-10-14T12:49:33Z

Closing this PR. The work continues in a separate series of PRs.

apwojcik · 2023-10-14T12:50:30Z

Please do not delete the uif2-initial branch!

apwojcik requested review from averinevg, cderb and JehandadKhan July 25, 2023 11:51

apwojcik self-assigned this Jul 25, 2023

apwojcik added the complexity_middle label Jul 25, 2023

apwojcik mentioned this pull request Jul 25, 2023

[Draft] Enable MIOpen on Windows #2156

Closed

apwojcik force-pushed the uif2-initial branch from 9a15b4a to 54e8a4a Compare July 25, 2023 12:25

averinevg requested a review from DrizztDoUrden July 25, 2023 13:07

averinevg requested changes Jul 25, 2023

View reviewed changes

CAHEK7 requested changes Jul 25, 2023

View reviewed changes

apwojcik force-pushed the uif2-initial branch 6 times, most recently from c3f8d52 to 42aebc4 Compare July 26, 2023 10:05

atamazov suggested changes Jul 26, 2023

View reviewed changes

apwojcik force-pushed the uif2-initial branch from 42aebc4 to 98531eb Compare July 26, 2023 22:06

apwojcik force-pushed the uif2-initial branch 2 times, most recently from 327cff9 to 9718bc5 Compare July 27, 2023 13:56

averinevg reviewed Jul 27, 2023

View reviewed changes

apwojcik force-pushed the uif2-initial branch 3 times, most recently from 8d1fdb1 to 009a461 Compare July 28, 2023 14:52

TeutonJon78 mentioned this pull request Jul 29, 2023

[Feature]: Add ROCm for Windows support vladmandic/automatic#1880

Open

4 tasks

apwojcik force-pushed the uif2-initial branch 2 times, most recently from 997446d to 4f7740f Compare August 24, 2023 10:07

njsharpe mentioned this pull request Aug 24, 2023

App startup error when running with ROCm/HIP 5.5 installed on Windows nod-ai/SHARK-Studio#1789

Closed

evshiron mentioned this pull request Sep 16, 2023

ROCM5.7 build pytorch failed evshiron/rocm_lab#14

Closed

enable compilation on Windows

8cb1a62

Signed-off-by: Artur Wojcik <artur.wojcik@amd.com> rocblas: disable beta features on Windows

apwojcik force-pushed the uif2-initial branch from 4f7740f to 8cb1a62 Compare September 26, 2023 18:59

regression: do not use file system symbolic links

6678e47

apwojcik force-pushed the uif2-initial branch from a4ea669 to 6678e47 Compare October 1, 2023 20:40

apwojcik added 4 commits October 1, 2023 22:40

Merge branch 'develop' into uif2-initial

031131d

fix compilation of speedtest_sequence

614a239

cmake: switch to NATIVE_COMMAND from UNIX_COMMAND

db4b383

fix speedtests sequences on Windows

a8f59c6

apwojcik closed this Oct 14, 2023

	# Copyright (c) 2017 Advanced Micro Devices, Inc.
	# Copyright (c) 2023 Advanced Micro Devices, Inc.

[DRAFT] enable MIOpen on Windows #2272

[DRAFT] enable MIOpen on Windows #2272

Conversation

apwojcik commented Jul 25, 2023

averinevg commented Jul 25, 2023

averinevg left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apwojcik Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Joly0 Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apwojcik Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atamazov left a comment

Choose a reason for hiding this comment

dmikushin commented Jul 27, 2023 • edited Loading

apwojcik commented Jul 27, 2023

CAHEK7 commented Jul 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apwojcik commented Jul 27, 2023 • edited Loading

johnnynunez commented Aug 31, 2023

MidnightKittenCat commented Sep 11, 2023 • edited Loading

scarsty commented Sep 11, 2023 • edited Loading

xTye commented Sep 18, 2023

johnnynunez commented Sep 26, 2023 • edited Loading

scarsty commented Sep 29, 2023 • edited Loading

MidnightKittenCat commented Sep 29, 2023 • edited Loading

scarsty commented Sep 29, 2023 • edited Loading

NeedsMoar commented Sep 30, 2023

scarsty commented Sep 30, 2023

NeedsMoar commented Oct 1, 2023

JehandadKhan commented Oct 11, 2023

scarsty commented Oct 12, 2023 • edited Loading

averinevg commented Oct 13, 2023

averinevg commented Oct 13, 2023

apwojcik commented Oct 14, 2023

apwojcik commented Oct 14, 2023 • edited Loading

averinevg left a comment •

edited

Loading

apwojcik Jul 25, 2023 •

edited

Loading

Joly0 Oct 12, 2023 •

edited

Loading

apwojcik Jul 26, 2023 •

edited

Loading

dmikushin commented Jul 27, 2023 •

edited

Loading

apwojcik commented Jul 27, 2023 •

edited

Loading

MidnightKittenCat commented Sep 11, 2023 •

edited

Loading

scarsty commented Sep 11, 2023 •

edited

Loading

johnnynunez commented Sep 26, 2023 •

edited

Loading

scarsty commented Sep 29, 2023 •

edited

Loading

MidnightKittenCat commented Sep 29, 2023 •

edited

Loading

scarsty commented Sep 29, 2023 •

edited

Loading

scarsty commented Oct 12, 2023 •

edited

Loading

apwojcik commented Oct 14, 2023 •

edited

Loading