embed.cmake: add support for Windows resource file #2330

apwojcik · 2023-10-13T13:41:43Z

This PR introduces the support of Windows resource files to Embed.cmake. It is ON by default on Windows, and when it is OFF *.cpp files will be used. The same applies to Linux - ON -> *.o (LD) or OFF -> *.cpp .

This PR fixes building resources on Linux with ld and objcopy commands.

codecov · 2023-10-13T14:53:50Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@b249fb8). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 2ecbe79 differs from pull request most recent head c49352e. Consider uploading reports for the commit c49352e to get more accurate results

@@            Coverage Diff             @@
##             develop    #2330   +/-   ##
==========================================
  Coverage           ?   91.36%           
==========================================
  Files              ?      440           
  Lines              ?    16547           
  Branches           ?        0           
==========================================
  Hits               ?    15118           
  Misses             ?     1429           
  Partials           ?        0

migraphx-bot · 2023-10-13T22:23:55Z

Test	Batch	Rate new 4f44fd	Rate old 35e529	Diff	Compare
torchvision-resnet50	64	2,830.01	2,831.22	-0.04%	✅
torchvision-resnet50_fp16	64	6,492.37	6,496.79	-0.07%	✅
torchvision-densenet121	32	2,092.71	2,087.03	0.27%	✅
torchvision-densenet121_fp16	32	3,654.61	3,655.44	-0.02%	✅
torchvision-inceptionv3	32	1,582.89	1,586.48	-0.23%	✅
torchvision-inceptionv3_fp16	32	2,572.72	2,566.01	0.26%	✅
cadene-inceptionv4	16	703.13	703.21	-0.01%	✅
cadene-resnext64x4	16	692.54	691.96	0.08%	✅
slim-mobilenet	64	8,329.90	8,324.38	0.07%	✅
slim-nasnetalarge	64	225.47	225.38	0.04%	✅
slim-resnet50v2	64	2,664.93	2,666.92	-0.07%	✅
bert-mrpc-onnx	8	820.87	820.80	0.01%	✅
bert-mrpc-tf	1	388.02	389.08	-0.27%	✅
pytorch-examples-wlang-gru	1	298.88	298.91	-0.01%	✅
pytorch-examples-wlang-lstm	1	313.02	314.11	-0.35%	✅
torchvision-resnet50_1	1	602.35	604.06	-0.28%	✅
torchvision-inceptionv3_1	1	339.25	342.20	-0.86%	✅
cadene-dpn92_1	1	399.97	398.56	0.35%	✅
cadene-resnext101_1	1	329.03	329.05	-0.01%	✅
slim-vgg16_1	1	458.42	459.88	-0.32%	✅
slim-mobilenet_1	1	2,143.57	2,125.43	0.85%	✅
slim-inceptionv4_1	1	219.84	220.40	-0.25%	✅
onnx-taau-downsample	1	303.85	304.29	-0.15%	✅
dlrm-criteoterabyte	1	21.60	21.61	-0.05%	✅
dlrm-criteoterabyte_fp16	1	40.65	40.62	0.06%	✅
agentmodel	1	nan	nan	nan%	❌
unet_fp16	2	54.95	54.90	0.10%	✅
resnet50v1_fp16	1	946.48	964.66	-1.88%	✅
bert_base_cased_fp16	64	967.67	967.69	-0.00%	✅
bert_large_uncased_fp16	32	304.05	304.17	-0.04%	✅
bert_large_fp16	1	166.88	167.04	-0.10%	✅
distilgpt2_fp16	16	1,272.87	1,272.68	0.02%	✅

This build is not recommended to merge 🔴

migraphx-bot · 2023-10-13T22:23:57Z

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

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

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

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

:white_check_mark:torchvision-resnet50_1: PASSED: MIGraphX meets tolerance

:white_check_mark:torchvision-inceptionv3_1: PASSED: MIGraphX meets tolerance

:white_check_mark:cadene-dpn92_1: PASSED: MIGraphX meets tolerance

:white_check_mark:cadene-resnext101_1: PASSED: MIGraphX meets tolerance

:white_check_mark:slim-vgg16_1: PASSED: MIGraphX meets tolerance

:white_check_mark:slim-mobilenet_1: PASSED: MIGraphX meets tolerance

:white_check_mark:slim-inceptionv4_1: PASSED: MIGraphX meets tolerance

:white_check_mark:dlrm-criteoterabyte: PASSED: MIGraphX meets tolerance

❌agentmodel: ERROR - check error output

Traceback (most recent call last):
File "/src/AMDMIGraphX/tools/accuracy/accuracy_checker.py", line 336, in
main()
File "/src/AMDMIGraphX/tools/accuracy/accuracy_checker.py", line 254, in main
pred_migx = np.array(model.run(params)[-1])
RuntimeError: /src/AMDMIGraphX/src/targets/gpu/device/include/migraphx/gpu/device/visit.hpp:140: hip_visit_views_impl: Ranks must be the same

:white_check_mark:unet: PASSED: MIGraphX meets tolerance

:white_check_mark:resnet50v1: PASSED: MIGraphX meets tolerance

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

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

:white_check_mark:bert_large: PASSED: MIGraphX meets tolerance

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

pfultz2 · 2023-10-16T16:58:21Z

cmake/Embed.cmake


-option(EMBED_USE_LD "Use ld to embed data files" OFF)


We shouldn't remove this option.

The technique with LD on Linux is the same as RC on Windows. I changed EMBED_USE_LD to EMBED_USE_RESOURCES because, in both cases, the literature refers to both techniques as embedding resource files in program binary. Here is the example: https://atwillys.de/content/cc/embedding-resource-files-in-a-c-plus-plus-program-binary-on-linux-unix/?lang=en. I can easily find tonnes more. We only need one option to control resources or *.cpp files. It makes no sense to have both and expose EMBED_USE_LD on Windows and EMBOED_USE_RC on Linux.

The technique with LD on Linux is the same as RC on Windows. I changed EMBED_USE_LD to EMBED_USE_RESOURCES because, in both cases, the literature refers to both techniques as embedding resource files in program binary.

The name is the technique used for embedding the resource. We are embedding the resource when we use the C arrays as well, so the EMBED_USE_RESOURCES flag would apply there as well, so it doesnt make sense.

It makes no sense to have both and expose EMBED_USE_LD on Windows and EMBOED_USE_RC on Linux.

I would prefer the two flags because there are other techniques to embed resources as well(such as using #embed) so I would prefer to have a flag to specify the technique being used to embed the resources.

I prefer a single option for LD and RC (e.g., EMBED_USE_BINARY or EMBED_USE_NATIVE or EMBED_USE_OS_NATIVE) and separate for #embed when we switch to C++23 (e.g. EMBED_USE_CXX_EMBED or EMBED_USE_CXX or EMBED_USE_CXX_NATIVE) or something like that. Two flags for LD and RC would result in different Linux and Windows building instructions. One cannot use LD on Windows and RC on Linux. I prefer to keep them the same for both OSes.

We should probably have one EMBED_USE variable that can be set to EMBED_USE=RC for windows resource files, EMBED_USE=LD for gnu linker, and EMBED_USE=CArrays for the carrays version.

We can also use set_property(CACHE EMBED_USE PROPERTY STRINGS "LD;CArrays") to set the options for linux, so in cmake gui(or tui) you can select the option you want.

pfultz2 · 2023-10-16T16:58:59Z

cmake/Embed.cmake

+endif()
+
+if(NOT WIN32 AND EMBED_USE_RESOURCES)
+    find_program(EMBED_LD ld)


Why are we searching for ld when EMBED_USE_RESOURCES is enabled on linux? This should be an error.

Because one may turn it on Linux, see my comment above.

pfultz2 · 2023-10-16T17:00:14Z

cmake/Embed.cmake

 endfunction()

 function(add_embed_library EMBED_NAME)
-    set(options)
-    set(oneValueArgs RELATIVE)
-    set(multiValueArgs)


Dont remove these extra variables.

pfultz2 · 2023-10-16T17:01:27Z

cmake/Embed.cmake

-    set(oneValueArgs RELATIVE)
-    set(multiValueArgs)
-    cmake_parse_arguments(PARSE "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN})
+    set(oneValueArgs BASE_DIRECTORY)


Why is RELATIVE removed? We need to specify which directory we want the paths to relative to.

I changed RELATIVE to BASE_DIRECTORY to align with the cmake_path() command naming to make it cleaner to read cmake_path(RELATIVE_PATH <path-var> BASE_DIRECTORY <input> ...).

It uses RELATIVE to match file(GLOB) command. I dont think cmake_path makes sense here.

I changed file(GLOB) to cmake_path() for making relative paths. However, I don't care much about the name. If that is the only obstacle blocking this PR and you want the name to be RELATIVE, I make it RELATIVE again.

This PR introduces the support of Windows resource files to Embed.cmake. It is ON by default on Windows, and when it is OFF *.cpp files will be used. The same applies to Linux - ON -> *.o (LD) or OFF -> *.cpp . This PR fixes building resources on Linux with ld and objcopy commands.

embed.cmake: add support for Windows resource file

566bd2f

apwojcik added the Windows Related changes for Windows Environments label Oct 13, 2023

apwojcik requested review from pfultz2 and umangyadav October 13, 2023 13:41

pfultz2 reviewed Oct 16, 2023

View reviewed changes

apwojcik added 4 commits October 27, 2023 15:51

Merge branch 'develop' into embed_cmake_windows

57149a0

incorporate feedback after review

76ebf5a

Merge branch 'develop' into embed_cmake_windows

ea64199

fix compilation on SLES

7d51c82

apwojcik requested a review from pfultz2 October 28, 2023 17:18

apwojcik and others added 13 commits October 29, 2023 01:50

convert tabs into spaces

b0dc9d5

incorporate review feedback

694044e

Merge branch 'develop' into embed_cmake_windows

e4b71ad

incorporate review feedback

d98a0fc

Merge branch 'develop' into embed_cmake_windows

50c7e11

fix compile on Windows

c49352e

fix Windows paths in resource script file

6666984

Merge branch 'develop' into embed_cmake_windows

1c29d9a

Merge branch 'develop' into embed_cmake_windows

d4d80c7

Merge branch 'develop' into embed_cmake_windows

fb01a1f

Merge branch 'develop' into embed_cmake_windows

a255850

fix missing function migraphx_kernels()

bff17e2

Merge branch 'develop' into embed_cmake_windows

4f44fd0

pfultz2 approved these changes Nov 21, 2023

View reviewed changes

causten added 2 commits November 21, 2023 15:11

Merge branch 'develop' into embed_cmake_windows

0f4dd40

Merge branch 'develop' into embed_cmake_windows

cd22434

causten merged commit 2414885 into develop Nov 21, 2023
14 of 15 checks passed

causten deleted the embed_cmake_windows branch November 21, 2023 20:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

embed.cmake: add support for Windows resource file #2330

embed.cmake: add support for Windows resource file #2330

apwojcik commented Oct 13, 2023 •

edited

Loading

codecov bot commented Oct 13, 2023 •

edited

Loading

migraphx-bot commented Oct 13, 2023 •

edited

Loading

migraphx-bot commented Oct 13, 2023 •

edited

Loading

pfultz2 Oct 16, 2023

apwojcik Oct 16, 2023

pfultz2 Oct 29, 2023

apwojcik Oct 30, 2023

pfultz2 Oct 30, 2023

apwojcik Nov 1, 2023

pfultz2 Oct 16, 2023

apwojcik Oct 16, 2023 •

edited

Loading

pfultz2 Oct 16, 2023

pfultz2 Oct 16, 2023

apwojcik Oct 16, 2023 •

edited

Loading

pfultz2 Oct 29, 2023

apwojcik Oct 30, 2023

embed.cmake: add support for Windows resource file #2330

embed.cmake: add support for Windows resource file #2330

Conversation

apwojcik commented Oct 13, 2023 • edited Loading

codecov bot commented Oct 13, 2023 • edited Loading

Codecov Report

migraphx-bot commented Oct 13, 2023 • edited Loading

migraphx-bot commented Oct 13, 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

apwojcik Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apwojcik Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apwojcik commented Oct 13, 2023 •

edited

Loading

codecov bot commented Oct 13, 2023 •

edited

Loading

migraphx-bot commented Oct 13, 2023 •

edited

Loading

migraphx-bot commented Oct 13, 2023 •

edited

Loading

apwojcik Oct 16, 2023 •

edited

Loading

apwojcik Oct 16, 2023 •

edited

Loading