Fix targets registration on Windows #2866

apwojcik · 2024-03-07T15:20:03Z

Unfortunately, automatic target registration does not work on Windows.
The linker will not add a specified DLL as a dependency if at least a symbol from the dynamic library is not used. Therefore, linking to migrpahx_all_targets does not link dynamic libraries on Windows as intended.

This PR changes automatic registration to runtime manual registration. Registration is done when the target library is loaded by the register_target() function exported from the target library. It is called from the migraphx::make_target() function.

pfultz2 · 2024-03-07T17:30:00Z

Can we just use the -Wl,--no-as-needed flag instead on windows?

pfultz2 · 2024-03-07T17:44:43Z

Also, we could make migraphx_all_targets a static library and then have it load the all targets before main:

__attribute__((constructor)) void load_targets()
{
            make_target("ref");
    #ifdef HAVE_CPU
            make_target("cpu");
    #endif
    #ifdef HAVE_GPU
            make_target("gpu");
    #endif
    #ifdef HAVE_FPGA
            make_target("fpga");
    #endif
}

apwojcik · 2024-03-07T21:00:21Z

Can we just use the -Wl,--no-as-needed flag instead on windows?

It is not supported on Windows. It is only applicable to shared objects.

apwojcik · 2024-03-07T21:10:51Z

Also, we could make migraphx_all_targets a static library and then have it load the all targets before main:

__attribute__((constructor)) void load_targets()
{
            make_target("ref");
    #ifdef HAVE_CPU
            make_target("cpu");
    #endif
    #ifdef HAVE_GPU
            make_target("gpu");
    #endif
    #ifdef HAVE_FPGA
            make_target("fpga");
    #endif
}

This will now work on Windows. DLLs are not the same as shared objects. Furthermore, MSVC/Clang on Windows does not recognize the constructor attribute because Windows does not support it. One must implement DllMain() to serve a similar purpose. However, that is more complicated because it is process-attached and thread-attached aware—the auto registration of a target creates thread-local storage, making it unavailable after exiting from a library loading thread. A DLL on Windows should generate and return an execution context to a caller to store the per-process state.

apwojcik · 2024-03-07T21:11:22Z

I managed to simplify the PR and limit the number of required changes.

codecov · 2024-03-07T22:22:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.97%. Comparing base (354bc02) to head (165d0ec).
Report is 141 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #2866   +/-   ##
========================================
  Coverage    91.97%   91.97%           
========================================
  Files          489      489           
  Lines        19390    19390           
========================================
  Hits         17833    17833           
  Misses        1557     1557

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

migraphx-bot · 2024-03-08T03:05:35Z

Test	Batch	Rate new 165d0e	Rate old 206e0f	Diff	Compare
torchvision-resnet50	64	1,741.50	1,750.99	-0.54%	✅
torchvision-resnet50_fp16	64	4,065.27	4,085.88	-0.50%	✅
torchvision-densenet121	32	1,458.17	1,465.87	-0.52%	✅
torchvision-densenet121_fp16	32	2,516.42	2,524.65	-0.33%	✅
torchvision-inceptionv3	32	885.24	890.07	-0.54%	✅
torchvision-inceptionv3_fp16	32	1,477.42	1,483.80	-0.43%	✅
cadene-inceptionv4	16	410.35	412.53	-0.53%	✅
cadene-resnext64x4	16	417.51	419.70	-0.52%	✅
slim-mobilenet	64	3,986.60	4,007.04	-0.51%	✅
slim-nasnetalarge	64	100.54	101.00	-0.46%	✅
slim-resnet50v2	64	1,671.94	1,681.03	-0.54%	✅
bert-mrpc-onnx	8	612.66	615.98	-0.54%	✅
bert-mrpc-tf	1	276.92	279.26	-0.84%	✅
pytorch-examples-wlang-gru	1	321.22	324.15	-0.90%	✅
pytorch-examples-wlang-lstm	1	290.98	327.92	-11.26%	🔴
torchvision-resnet50_1	1	467.81	469.45	-0.35%	✅
cadene-dpn92_1	1	246.86	247.24	-0.15%	✅
cadene-resnext101_1	1	203.25	203.51	-0.13%	✅
onnx-taau-downsample	1	205.51	206.27	-0.37%	✅
dlrm-criteoterabyte	1	22.82	22.91	-0.39%	✅
dlrm-criteoterabyte_fp16	1	42.56	42.74	-0.43%	✅
agentmodel	1	6,340.55	6,392.31	-0.81%	✅
unet_fp16	2	34.04	34.21	-0.49%	✅
resnet50v1_fp16	1	585.28	597.26	-2.01%	✅
resnet50v1_int8	1	577.83	572.90	0.86%	✅
bert_base_cased_fp16	64	642.42	646.37	-0.61%	✅
bert_large_uncased_fp16	32	197.82	199.03	-0.61%	✅
bert_large_fp16	1	116.83	117.51	-0.58%	✅
distilgpt2_fp16	16	1,204.72	1,211.72	-0.58%	✅
yolov5s	1	300.61	302.25	-0.54%	✅
tinyllama	1	23.21	23.34	-0.55%	✅
vicuna-fastchat	1	133.45	134.18	-0.54%	✅
whisper-tiny-encoder	1	243.16	244.17	-0.41%	✅
whisper-tiny-decoder	1	255.26	256.68	-0.55%	✅

This build is not recommended to merge 🔴

migraphx-bot · 2024-03-08T03:05:36Z

✅ bert-mrpc-onnx: PASSED: MIGraphX meets tolerance

✅ bert-mrpc-tf: PASSED: MIGraphX meets tolerance

✅ pytorch-examples-wlang-gru: PASSED: MIGraphX meets tolerance

✅ pytorch-examples-wlang-lstm: PASSED: MIGraphX meets tolerance

✅ torchvision-resnet50_1: PASSED: MIGraphX meets tolerance

✅ cadene-dpn92_1: PASSED: MIGraphX meets tolerance

✅ cadene-resnext101_1: PASSED: MIGraphX meets tolerance

✅ dlrm-criteoterabyte: PASSED: MIGraphX meets tolerance

✅ agentmodel: PASSED: MIGraphX meets tolerance

✅ unet: PASSED: MIGraphX meets tolerance

✅ resnet50v1: PASSED: MIGraphX meets tolerance

✅ bert_base_cased_fp16: PASSED: MIGraphX meets tolerance

🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output

✅ bert_large: PASSED: MIGraphX meets tolerance

✅ yolov5s: PASSED: MIGraphX meets tolerance

✅ tinyllama: PASSED: MIGraphX meets tolerance

✅ vicuna-fastchat: PASSED: MIGraphX meets tolerance

✅ whisper-tiny-encoder: PASSED: MIGraphX meets tolerance

✅ whisper-tiny-decoder: PASSED: MIGraphX meets tolerance

✅ distilgpt2_fp16: PASSED: MIGraphX meets tolerance

pfultz2 · 2024-03-08T17:47:51Z

Furthermore, MSVC/Clang on Windows does not recognize the constructor attribute because Windows does not support it.

Sure, we can write it using standard C++ as well:

struct auto_load_targets
{
    auto_load_targets()
    {
        make_target("ref");
#ifdef HAVE_CPU
        make_target("cpu");
#endif
#ifdef HAVE_GPU
        make_target("gpu");
#endif
#ifdef HAVE_FPGA
        make_target("fpga");
#endif
    }
};

static auto load_targets = auto_load_targets();

And this does work on windows because that is how we auto register unit tests.

One must implement DllMain() to serve a similar purpose.

Why would we call DllMain? This is not run from a dll. This is a static library being linked into an executable.

apwojcik · 2024-05-08T18:27:20Z

@pfultz2 it is done

pfultz2 · 2024-05-16T14:09:45Z

src/register_target.cpp

+    }
+};
+[[maybe_unused]] static auto load_targets = auto_load_targets{};
+} // namespace


This needs to go into a static/object library that we only link in for the tests. This prevents lazy loading the targets.

pfultz2 · 2024-05-16T14:12:00Z

src/register_target.cpp

@@ -44,8 +66,6 @@ std::unordered_map<std::string, target>& target_map()
    return m;
 }

-void register_target_init() { (void)target_map(); }


Dont remove this, I think this was necessary to ensure targets are loaded in some cases.

pfultz2 · 2024-05-16T14:12:44Z

src/include/migraphx/register_target.hpp

@@ -66,9 +65,6 @@ struct register_target_action
    }
 };

-template <class T>
-using auto_register_target = auto_register<register_target_action, T>;


Dont remove this, We still want to provide the option to register a target with CRTP.

apwojcik · 2024-05-28T13:02:19Z

@causten CodeCov reposts a 0.01% test coverage decrease. However, when I look into the pull request details, it says that the patch is 100% covered. Am I reading it wrong?

pfultz2 · 2024-06-06T13:36:52Z

test/CMakeLists.txt

@@ -33,7 +33,11 @@ if(MIGRAPHX_DISABLE_LARGE_BUFFER_TESTS)
    add_compile_definitions(MIGRAPHX_DISABLE_LARGE_BUFFER_TESTS)
 endif()

+add_library(register_targets STATIC register_target.cpp)
+target_link_libraries(register_targets PRIVATE migraphx)


I think you need to link migraphx_all_targets here as well to get the macros defines such as HAVE_GPU, HAVE_CPU, etc.

pfultz2 · 2024-06-06T13:44:58Z

src/register_target.cpp

@@ -86,16 +84,5 @@ target make_target(const std::string& name)
    return it->second;
 }

-std::vector<std::string> get_targets()


This function should not be removed.

Something is not right; I reverted that change. I will double-check.

I fixed it.

fix target registering on Windows

34bf488

apwojcik added the Windows Related changes for Windows Environments label Mar 7, 2024

apwojcik requested review from pfultz2, umangyadav and varunsh-xilinx March 7, 2024 15:20

apwojcik requested a review from causten as a code owner March 7, 2024 15:20

upgrade the year in licenses

fc29bd6

[email protected] and others added 6 commits March 7, 2024 19:30

fix compilation of the fpga target

cd46541

update the year in CMakeLists.txt files

60860e9

fix cppcheck reports

7de05db

undo *requirements.txt

bbe1d3d

Merge branch 'develop' into register_target

6b14d6d

minimize the number of changes required

1e1f4fb

apwojcik added 2 commits March 7, 2024 23:38

add missing migraphx_all_targets to tests

07cc374

update the year in the license

4624e43

apwojcik added 4 commits April 23, 2024 21:31

Merge branch 'develop' into register_target

151dac7

Merge branch 'develop' into register_target

894d40f

incorporate review feedback

45ede6f

fix typo

caac2b0

fix clang-format

0e73fdb

pfultz2 reviewed May 16, 2024

View reviewed changes

apwojcik added the UAI label May 27, 2024

apwojcik added 7 commits May 27, 2024 20:24

Merge branch 'develop' into register_target

e1f6e2c

incorporate review feedback

9d8bcf8

Merge branch 'develop' into register_target

5e3411f

incorporate review feedback

db78e91

fix license check

5a235c1

fix clang-format

6ec5a5e

incorporate review feedback

31459c7

apwojcik requested a review from pfultz2 May 27, 2024 21:41

apwojcik added 5 commits May 29, 2024 00:34

Merge branch 'develop' into register_target

75a5b61

Merge branch 'develop' into register_target

bbf93b0

Merge branch 'develop' into register_target

52fc7d1

Merge branch 'develop' into register_target

2ad56ac

Merge branch 'develop' into register_target

f00dc28

pfultz2 reviewed Jun 6, 2024

View reviewed changes

apwojcik added 3 commits June 6, 2024 15:38

fix target library registration on Windows

a3ccae5

add missing linking to migraphx_all_tragets

2004b73

Merge branch 'develop' into register_target

14bf216

pfultz2 reviewed Jun 6, 2024

View reviewed changes

apwojcik added 2 commits June 6, 2024 16:19

revert some more changes

bcd8ad1

revert some more changes

165d0ec

pfultz2 approved these changes Jun 6, 2024

View reviewed changes

causten merged commit 102a0a4 into develop Jun 6, 2024
43 checks passed

causten deleted the register_target branch June 6, 2024 18:11

causten pushed a commit that referenced this pull request Jun 26, 2024

Fix targets registration on Windows (#2866)

f997bb2

lajagapp pushed a commit to lajagapp/AMDMIGraphX that referenced this pull request Jul 8, 2024

Fix targets registration on Windows (ROCm#2866)

4ea4930

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix targets registration on Windows #2866

Fix targets registration on Windows #2866

apwojcik commented Mar 7, 2024

pfultz2 commented Mar 7, 2024

pfultz2 commented Mar 7, 2024 •

edited

Loading

apwojcik commented Mar 7, 2024

apwojcik commented Mar 7, 2024

apwojcik commented Mar 7, 2024

codecov bot commented Mar 7, 2024 •

edited

Loading

migraphx-bot commented Mar 8, 2024 •

edited

Loading

migraphx-bot commented Mar 8, 2024 •

edited

Loading

pfultz2 commented Mar 8, 2024

apwojcik commented May 8, 2024

pfultz2 May 16, 2024

pfultz2 May 16, 2024

pfultz2 May 16, 2024

apwojcik commented May 28, 2024

pfultz2 Jun 6, 2024

apwojcik Jun 6, 2024

pfultz2 Jun 6, 2024

apwojcik Jun 6, 2024

apwojcik Jun 6, 2024

Fix targets registration on Windows #2866

Fix targets registration on Windows #2866

Conversation

apwojcik commented Mar 7, 2024

pfultz2 commented Mar 7, 2024

pfultz2 commented Mar 7, 2024 • edited Loading

apwojcik commented Mar 7, 2024

apwojcik commented Mar 7, 2024

apwojcik commented Mar 7, 2024

codecov bot commented Mar 7, 2024 • edited Loading

Codecov Report

migraphx-bot commented Mar 8, 2024 • edited Loading

migraphx-bot commented Mar 8, 2024 • edited Loading

pfultz2 commented Mar 8, 2024

apwojcik commented May 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apwojcik commented May 28, 2024

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

pfultz2 commented Mar 7, 2024 •

edited

Loading

codecov bot commented Mar 7, 2024 •

edited

Loading

migraphx-bot commented Mar 8, 2024 •

edited

Loading

migraphx-bot commented Mar 8, 2024 •

edited

Loading