[Windows] Build applications tests and benchmarks on Windows [Include #2924 changes] #2952

gkisalapl · 2025-02-14T08:12:16Z

This PR add possibility to run soma applications, tests and benchmarks on windows
It also fix support for blas and avx on windows

Self-evaluation:

Build test: [X]Passed [ ]Failed [ ]Skipped
Run test: [X]Passed [ ]Failed [ ]Skipped

dkjung · 2025-02-14T08:24:30Z

You could make two PRs if you had two features. That would help others review. Just let this as it is for now.
I'll see it, thanks!

dkjung

LGTM

gkisalapl · 2025-02-17T11:29:40Z

You could make two PRs if you had two features. That would help others review. Just let this as it is for now. I'll see it, thanks!

I originally make this changes in 2 PRs but first one is in review for more than week (see #2924)

djeong20

Awesome! can't wait to dive into benchmarking on Windows :)

djeong20 · 2025-02-18T02:17:11Z

third_party/benchmark.wrap

@@ -0,0 +1,4 @@
+[wrap-git]


Please move *wrap files under the subprojects directory. (#2917)

Please move *wrap files under the subprojects directory. (#2917)

Ok

EunjuYang · 2025-02-18T08:43:19Z

You could make two PRs if you had two features. That would help others review. Just let this as it is for now. I'll see it, thanks!

I originally make this changes in 2 PRs but first one is in review for more than week (see #2924)

For the case, please mention the dependency between PRs in your title. This will be helpful for reviewers!
For example, add [Wait for #2924] at the beginning of your title!

piotrrak · 2025-02-19T02:18:54Z

Applications/AlexNet/jni/meson.build


-nntr_alex_resdir = nntr_app_resdir / 'AlexNet'
-run_command('cp', '-lr', res_path, nntr_alex_resdir)
+if host_machine.system() == 'windows'


You don't need that, it can be done in native file.
Please see https://mesonbuild.com/Machine-files.html#binaries
and also my POC

[binaries] #mkdir = ['mkdir'] mkdir = ['wine', 'cmd.exe', '/c','mkdir'] nproc = ['wine', 'cmd', '/c', '@echo', '%NUMBER_OF_PROCESSORS%'] touch = ['@DIRNAME@'/'touch.bat']

It'll require find_program('cp')

piotrrak · 2025-02-19T02:26:35Z

meson.build

-      add_project_arguments(['-mavx2'], language: ['c','cpp'])
-      message('-march=native added for AVX hardware acceleration.')
+      if host_machine.system() == 'windows'
+        add_project_arguments(['/arch:AVX2'], language: ['c','cpp'])


Also possible in native-file
cpp_args=...
cpp_link_args=...
we still need if != windows`

piotrrak · 2025-02-19T02:28:35Z

meson.build

-run_command('mkdir', '-p', nntrainer_resdir)
+
+if host_machine.system() == 'windows'
+  nntrainer_resdir_win = nntrainer_resdir.replace('/', '\\')


Can't we construct it correctly using '/'?
Couldn't test with wine since Z:\ is required for abs path.

piotrrak · 2025-02-19T02:30:29Z

meson.build

+      blas_options.add_cmake_defines({'BUILD_TESTING': false})
+      blas_options.add_cmake_defines({'BUILD_BENCHMARKS': false})
+      blas_options.add_cmake_defines({'BUILD_SHARED_LIBS': false})
+      blas_options.add_cmake_defines({'BUILD_WITHOUT_LAPACK': true})
+      blas_subproject = cmake.subproject('openblas', options: blas_options, required: true)


Didn't ever test it but worth checking if defining those in native [cmake] section works

piotrrak · 2025-02-19T02:30:53Z

meson.build

+  if not benchmark_dep.found()
+    benchmark_options = cmake.subproject_options()
+    if host_machine.system() == 'windows'
+      benchmark_options.append_compile_args('c', windows_compile_args)


Again native file

piotrrak · 2025-02-19T02:31:32Z

meson.build

-libm_dep = cxx.find_library('m') # cmath library
-libdl_dep = cxx.find_library('dl') # DL library
+if host_machine.system() != 'windows'
+  libm_dep = cxx.find_library('m') # cmath library


required: false should do

piotrrak · 2025-02-19T02:31:44Z

meson.build

-libdl_dep = cxx.find_library('dl') # DL library
+if host_machine.system() != 'windows'
+  libm_dep = cxx.find_library('m') # cmath library
+  libdl_dep = cxx.find_library('dl') # DL library


piotrrak · 2025-02-19T02:32:54Z

meson.build

 if get_option('platform') == 'android'
-  iniparser_root = meson.source_root() / 'third_party' / 'iniparser'
+  iniparser_root = meson.source_root() / 'subprojects' / 'iniparser'
  iniparser_dep = declare_dependency()
 else
  iniparser_dep = dependency('iniparser', required : false, version : '>=3.2') # iniparser


Maybe we could use `dependency(..., fallback='iniparse')

piotrrak · 2025-02-19T02:35:45Z

meson.build

-    libcurl_dep = dependency('libcurl')
-    if not tflite_dep.found()
-      error('Tensorflow-Lite dependency not found')
+    if host_machine.system() != 'windows'


We might want to make it optional for all platforms in 'Applications', I already do to some degree for openvino.
Warn that something-something is unsupported

piotrrak · 2025-02-19T02:36:54Z

meson.build

    endif
    subdir('Applications')
  endif
 endif

-if get_option('platform') != 'android'
+if get_option('platform') != 'android' and host_machine.system() != 'windows'


We should use get_option() here really, since it's already optional by meson.options

piotrrak · 2025-02-19T02:38:03Z

nntrainer/app_context.cpp

@@ -207,15 +207,17 @@ std::vector<std::string> getPluginPaths() {
 const std::string getFullPath(const std::string &path,
                              const std::string &base) {
  /// if path is absolute, return path
-  if (path[0] == '/') {
+  if (std::filesystem::path(path).is_absolute()) {


maybe global alias since it is *.cpp

namespace fs = std::filesystem;

What I already do.

piotrrak · 2025-02-19T02:38:49Z

nntrainer/cl_context.cpp

@@ -231,12 +231,12 @@ bool ClContext::clCreateKernel(std::string &kernel_string,
      size_t binary_size = fs.tellg();
      fs.seekg(0, std::ios::beg);

-      unsigned char chunk[binary_size];
-      fs.read((char *)chunk, binary_size);
+      std::vector<unsigned char> chunk(binary_size);


Brace constructor? Nitpick

piotrrak · 2025-02-19T02:39:05Z

nntrainer/layers/cl_layers/reshape_cl.cpp

@@ -163,6 +163,7 @@ void ReshapeLayerCl::ReshapeProcess(Tensor const &input, Tensor &output) {
  }
 }

+#ifdef ENABLE_FP16


piotrrak · 2025-02-19T02:39:35Z

nntrainer/meson.build

  thread_dep,
  openmp_dep
 ]

+if host_machine.system() != 'windows'


required:false should do it

piotrrak · 2025-02-19T02:40:30Z

nntrainer/meson.build

@@ -101,9 +105,13 @@ else
    install_dir: nntrainer_libdir
  )

-  nntrainer_lib = nntrainer_shared
-  if get_option('default_library') == 'static'
+  if host_machine.system() == 'windows'


Comment? What is going on here?

Maybe we should ban default_library=shared build on windows, or override it in native file and warn/error at toplevel

piotrrak · 2025-02-19T02:42:55Z

nntrainer/tensor/blas_avx.cpp

+#if defined(_WIN32)
+    __m256 temp = _mm256_loadu_ps(&X[i]);
+    _mm256_storeu_ps(&Y[i], temp);
+#else
    __asm__ __volatile__("vmovups (%1), %%ymm0\n\t"
                         "vmovups %%ymm0, (%0)\n\t"
                         :
                         : "r"(&Y[i]), "r"(&X[i])
                         : "ymm0", "memory");
+#endif


Wonder if we really wan't inline asm here.

Should be same modulo forcing ymm0 here.

Current nntrainer is undergoing major refactor process on blas-level codes, so it is highly recommended to remove any fixes at tensor/blas_* files if not super urgent.

piotrrak

Didn't finish this review, but looks good, probably will rebase my changes.

piotrrak · 2025-02-19T02:45:46Z

Applications/Layers/jni/meson.build

@@ -1,8 +1,15 @@
-build_root = meson.build_root()
-res_path = meson.current_source_dir() / '..' / 'res'
+fs = import('fs')


Please move closer to begin of file, it'll be useful
It's include/import as in other language.

gkisalapl · 2025-02-19T11:02:51Z

Didn't finish this review, but looks good, probably will rebase my changes.

I'll add proposed modifications in next PR

piotrrak · 2025-02-19T17:24:46Z

@gkisalapl fine by me, thanks

This PR add possibility to run soma applications, tests and benchmarks on windows It aslo fix support for blas and avx on windows **Self-evaluation:** 1. Build test: [X]Passed [ ]Failed [ ]Skipped 2. Run test: [X]Passed [ ]Failed [ ]Skipped Signed-off-by: Grzegorz Kisala <[email protected]>

github-actions bot added the Need Review label Feb 14, 2025

gkisalapl changed the title ~~Apps tests windows~~ [Windows] Build applications tests and benchmarks windows Feb 14, 2025

dkjung approved these changes Feb 14, 2025

View reviewed changes

gkisalapl mentioned this pull request Feb 14, 2025

[windows] Add experimental windows build #2868

Closed

gkisalapl force-pushed the apps_tests_windows branch 4 times, most recently from 1db2541 to 17aaadc Compare February 17, 2025 11:27

gkisalapl changed the title ~~[Windows] Build applications tests and benchmarks windows~~ [Windows] Build applications tests and benchmarks Windows Feb 17, 2025

gkisalapl force-pushed the apps_tests_windows branch from 17aaadc to 64aa7cd Compare February 17, 2025 16:16

gkisalapl marked this pull request as ready for review February 17, 2025 21:48

gkisalapl requested review from myungjoo, jijoongmoon, again4you, jaeyun-jung, leemgs, wooksong, gichan-jang, anyj0527, lhs8928, songgot, jihochu, DonghakPark, SeoHyungjun, baek2sm, skykongkong8, djeong20 and EunjuYang as code owners February 17, 2025 21:48

gkisalapl requested a review from haehun as a code owner February 17, 2025 21:48

djeong20 approved these changes Feb 18, 2025

View reviewed changes

gkisalapl force-pushed the apps_tests_windows branch from 64aa7cd to 88ca77f Compare February 18, 2025 08:07

gkisalapl changed the title ~~[Windows] Build applications tests and benchmarks Windows~~ [Windows] Build applications tests and benchmarks Windows [include #2924 changes] Feb 18, 2025

gkisalapl changed the title ~~[Windows] Build applications tests and benchmarks Windows [include #2924 changes]~~ [Windows] Build applications tests and benchmarks on Windows [include #2924 changes] Feb 18, 2025

gkisalapl changed the title ~~[Windows] Build applications tests and benchmarks on Windows [include #2924 changes]~~ [Windows] Build applications tests and benchmarks on Windows [Include #2924 changes] Feb 18, 2025

piotrrak suggested changes Feb 19, 2025

View reviewed changes

piotrrak reviewed Feb 19, 2025

View reviewed changes

gkisalapl force-pushed the apps_tests_windows branch from 88ca77f to 51e9c8a Compare February 20, 2025 07:31

gkisalapl force-pushed the apps_tests_windows branch from 51e9c8a to 4dcf0a2 Compare February 20, 2025 09:52

dkjung requested a review from piotrrak February 25, 2025 07:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Windows] Build applications tests and benchmarks on Windows [Include #2924 changes] #2952

[Windows] Build applications tests and benchmarks on Windows [Include #2924 changes] #2952

gkisalapl commented Feb 14, 2025

dkjung commented Feb 14, 2025 •

edited

Loading

dkjung left a comment

gkisalapl commented Feb 17, 2025

djeong20 left a comment

djeong20 Feb 18, 2025

gkisalapl Feb 18, 2025

EunjuYang commented Feb 18, 2025 •

edited

Loading

piotrrak Feb 19, 2025

piotrrak Feb 19, 2025

piotrrak Feb 19, 2025

piotrrak Feb 19, 2025

piotrrak Feb 19, 2025

piotrrak Feb 19, 2025

piotrrak Feb 19, 2025

piotrrak Feb 19, 2025

piotrrak Feb 19, 2025

piotrrak Feb 19, 2025

piotrrak Feb 19, 2025

piotrrak Feb 19, 2025

piotrrak Feb 19, 2025

piotrrak Feb 19, 2025

piotrrak Feb 19, 2025

piotrrak Feb 19, 2025

piotrrak Feb 19, 2025

piotrrak Feb 19, 2025

skykongkong8 Feb 25, 2025

piotrrak left a comment

piotrrak Feb 19, 2025

gkisalapl commented Feb 19, 2025

piotrrak commented Feb 19, 2025

[Windows] Build applications tests and benchmarks on Windows [Include #2924 changes] #2952

Are you sure you want to change the base?

[Windows] Build applications tests and benchmarks on Windows [Include #2924 changes] #2952

Conversation

gkisalapl commented Feb 14, 2025

dkjung commented Feb 14, 2025 • edited Loading

dkjung left a comment

Choose a reason for hiding this comment

gkisalapl commented Feb 17, 2025

djeong20 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EunjuYang commented Feb 18, 2025 • 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

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

Choose a reason for hiding this comment

piotrrak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkisalapl commented Feb 19, 2025

piotrrak commented Feb 19, 2025

dkjung commented Feb 14, 2025 •

edited

Loading

EunjuYang commented Feb 18, 2025 •

edited

Loading