Fix compile errors with MSVC(alternative to #904) #908

AnhBe0 · 2023-07-14T17:03:29Z

This PR fixes the errors we get when using MSVC:

MSVC crashes when doing SFINAE on the return type, so we changed the "{}" to "::value"
MSVC does not support M_PI, use Kokkos facility instead
replace constexpr with const in a few places

masterleinad · 2023-07-14T17:10:26Z

The indentation is off. How is this different from #904 and why did you choose to create a new pull request (instead of pushing to the other one)?

AnhBe0 · 2023-07-14T17:26:30Z

I just fixed the indentation. The different is for the SFINAE, to fix the error I use ""::value" instead of "{}".For the new pull request, Dr. Bruno told me to create it.

masterleinad

I like this better if we don't have to have different versions for NVCC and MSVC.

masterleinad · 2023-07-14T17:43:50Z

test/tstRay.cpp

@@ -172,7 +172,7 @@ BOOST_AUTO_TEST_CASE(intersects_box)
  {                                                                            \
    float t0;                                                                  \
    float t1;                                                                  \
-    constexpr auto inf = KokkosExt::ArithmeticTraits::infinity<float>::value;  \
+    auto const inf = KokkosExt::ArithmeticTraits::infinity<float>::value;      \


I would prefer if we can only make this const for MSVC and constexpr otherwise.

I agree with @masterleinad.

masterleinad · 2023-07-14T18:32:58Z

Retest this please.

dalg24

LGTM. I like this approach better than #904

src/ArborX_BruteForce.hpp

aprokop

Other than constexpr -> const, this looks fine. I also think we should switch all Kokkos::is_view<View>::value to Kokkos::is_view_v<View>.

I'm not sure about the constexpr. Is tstRay.cpp the only place it happens?

Rombur · 2023-07-16T23:35:53Z

Is tstRay.cpp the only place it happens?

Yes, that's the only place there is an issue. The others constexpr are fine. However, we didn't enable benchmark so there might be something wrong there too.

aprokop · 2023-07-17T13:39:27Z

However, we didn't enable benchmark so there might be something wrong there too.

So the assumption is that the users using MSVC would only be interested in examples? I think that's reasonable, though it would have been nice to be able to run benchmarks too.

Rombur · 2023-07-17T13:42:26Z

So the assumption is that the users using MSVC would only be interested in examples?

No it's just that getting Boost to work is already a total pain and we didn't have the courage to install google benchmark.

aprokop · 2023-07-19T15:27:23Z

I'm not sure I understand what's going on. My understanding was that @AnhBe0's patch was able to compile and run tests and examples, and only benchmarks were not enabled. However, I see that @masterleinad disabled some of the tests and examples.

masterleinad · 2023-07-19T15:31:04Z

I'm not sure I understand what's going on. My understanding was that @AnhBe0's patch was able to compile and run tests and examples, and only benchmarks were not enabled. However, I see that @masterleinad disabled some of the tests and examples.

Right, I am running into

Exit code 0xc0000135

for the disabled tests, see https://github.com/arborx/ArborX/actions/runs/5599297250/jobs/10240076970. It's not clear to me what that means but that was the best compromise I was able to come up with until now. Maybe, @AnhBe0 wants to investigate some more.

masterleinad · 2023-07-20T15:16:58Z

@AnhBe0 Why did you remove my commits adding GitHub CI?

AnhBe0 · 2023-07-20T15:34:56Z

I am sorry just added the #ifdef _MSVC_VER for the tstRay.cpp, did I push it in the wrong way ?

Rombur · 2023-07-20T16:07:49Z

test/tstRay.cpp

  constexpr Box unit_box{{0, 0, 0}, {1, 1, 1}};
-  constexpr auto inf = KokkosExt::ArithmeticTraits::infinity<float>::value;
+#endif
+  auto const inf = KokkosExt::ArithmeticTraits::infinity<float>::value;


This should still be constexpr when using linux

masterleinad · 2023-07-20T16:16:48Z

I am sorry just added the #ifdef _MSVC_VER for the tstRay.cpp, did I push it in the wrong way ?

You force-pushed squashing and overwriting all commits in this branch. I'll just push the commits again.

aprokop · 2023-07-20T19:22:00Z

test/tstRay.cpp

+// use const instead of constexpr because MSVC shows error(error:expression
+// must have a constant value)
+#ifdef _MSVC_VER
+  const Box unit_box{{0, 0, 0}, {1, 1, 1}};


An alternative would be to define a macro

#ifdef _MSVC_VER #define ARBORX_MSVC_CONSTEXPR const #else #define ARBORX_MSVC_CONSTEXPR constexpr #endif

and then do things like

ARBORX_MSVC_CONSTEXPR Box unit_box{{0, 0, 0}, {1, 1, 1}};

We looking at this more but it looks like we may not need to change this constexpr after all. We may only need to change the inf.

Rombur · 2023-08-01T14:24:05Z

test/tstRay.cpp

  using ArborX::Sphere;
  using ArborX::Experimental::Ray;
-
+  #ifdef _MSVC_VER


_MSVC_VERis never defined yet the code still works. This means this change is useless.

aprokop · 2023-08-07T14:50:22Z

We decided to simplify this PR to only be able to compile, but not run anything due to the difficulties of proper linking/runtime to the boost.

masterleinad · 2023-08-07T14:54:02Z

Please fix the merge conflicts.

masterleinad · 2023-08-07T21:12:21Z

Looks like there are still some problems in ArborX_Ray.hpp.

masterleinad · 2023-08-07T22:56:21Z

benchmark.lib(benchmark.cc.obj) : error LNK2038: mismatch detected for '_ITERATOR_DEBUG_LEVEL': value '0' doesn't match value '2' in bvh_driver.obj [D:\a\ArborX\ArborX\build\benchmarks\bvh_driver\ArborX_Benchmark_BoundingVolumeHierarchy.exe.vcxproj]

masterleinad · 2023-08-15T02:56:25Z

benchmarks/CMakeLists.txt

@@ -4,7 +4,9 @@ find_package(benchmark 1.5.4 REQUIRED)
 message(STATUS "Found benchmark: ${benchmark_DIR} (version \"${benchmark_VERSION}\")")

 add_subdirectory(brute_force_vs_bvh)
-add_subdirectory(bvh_driver)
+if(NOT WIN32)


Please add a FIXME with a suitable comment describing we this gets disabled here.

benchmarks/bvh_driver/benchmark_registration.hpp

benchmarks/distributed_tree_driver/distributed_tree_driver.cpp

Co-authored-by: Daniel Arndt <arndtd@ornl.gov>

masterleinad · 2023-08-15T13:03:39Z

benchmarks/CMakeLists.txt

+  # FIXME: for now, skip benchmarks using Google benchmark
+  # when building for Windows


That explains the "what" but not the "why".

Skip benchmarks that depend on Google Benchmark

benchmarks/bvh_driver/benchmark_registration.hpp

dalg24 · 2023-08-21T22:46:30Z

.github/workflows/windows.yml

+    - name: Build ArborX
+      shell: bash
+      run: |
+        cmake --build build --target install -- -m


We build but we don't actually run anything. Is that an oversight?

No we have problems with boost at runtime.

Just Boost.Test or also Boost.Program_options?

We had issue with both. They both need to be compiled

Note that #902 successfully ran tests.

Note that #902 successfully ran tests.

I don't understand that comment

I don't understand that comment

#902 had

- name: Build ArborX shell: bash run: | cmake --build build --target install -- -m cd build ctest -C Debug -V

and ran the tests (without enabling benchmarks).

AnhBe0 force-pushed the fixerror2 branch from 821e1b2 to 751da5f Compare July 14, 2023 17:20

masterleinad reviewed Jul 14, 2023

View reviewed changes

dalg24 reviewed Jul 14, 2023

View reviewed changes

src/ArborX_BruteForce.hpp Outdated Show resolved Hide resolved

aprokop reviewed Jul 15, 2023

View reviewed changes

AnhBe0 force-pushed the fixerror2 branch 2 times, most recently from d204c4b to 463f795 Compare July 17, 2023 14:27

AnhBe0 force-pushed the fixerror2 branch 3 times, most recently from f241c37 to f8ca0bb Compare July 20, 2023 15:09

Rombur reviewed Jul 20, 2023

View reviewed changes

aprokop reviewed Jul 20, 2023

View reviewed changes

Rombur reviewed Aug 1, 2023

View reviewed changes

aprokop force-pushed the fixerror2 branch 2 times, most recently from 4f1ae13 to 8560fdc Compare August 7, 2023 20:43

aprokop force-pushed the fixerror2 branch from 8560fdc to b5f924b Compare August 7, 2023 21:50

aprokop force-pushed the fixerror2 branch from b43678c to 602adf2 Compare August 14, 2023 23:35

masterleinad requested changes Aug 15, 2023

View reviewed changes

dalg24 reviewed Aug 15, 2023

View reviewed changes

benchmarks/bvh_driver/benchmark_registration.hpp Outdated Show resolved Hide resolved

benchmarks/distributed_tree_driver/distributed_tree_driver.cpp Outdated Show resolved Hide resolved

benchmarks/distributed_tree_driver/distributed_tree_driver.cpp Outdated Show resolved Hide resolved

AnhBe0 and others added 2 commits August 15, 2023 08:57

Fix complier errors on MSVC

f3d6237

Co-authored-by: Daniel Arndt <arndtd@ornl.gov>

Add Windows CI

5073fa6

aprokop force-pushed the fixerror2 branch 2 times, most recently from 872ccd8 to adb42b7 Compare August 15, 2023 12:59

masterleinad reviewed Aug 15, 2023

View reviewed changes

Reenable benchmarks

1147304

Skip benchmarks that depend on Google Benchmark

aprokop force-pushed the fixerror2 branch from adb42b7 to 1147304 Compare August 15, 2023 13:23

masterleinad approved these changes Aug 15, 2023

View reviewed changes

aprokop reviewed Aug 15, 2023

View reviewed changes

benchmarks/bvh_driver/benchmark_registration.hpp Show resolved Hide resolved

aprokop approved these changes Aug 15, 2023

View reviewed changes

aprokop merged commit cf40dc5 into arborx:master Aug 15, 2023
1 check passed

dalg24 reviewed Aug 21, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compile errors with MSVC(alternative to #904) #908

Fix compile errors with MSVC(alternative to #904) #908

AnhBe0 commented Jul 14, 2023

masterleinad commented Jul 14, 2023

AnhBe0 commented Jul 14, 2023

masterleinad left a comment

masterleinad Jul 14, 2023

aprokop Jul 18, 2023

masterleinad commented Jul 14, 2023

dalg24 left a comment

aprokop left a comment

Rombur commented Jul 16, 2023

aprokop commented Jul 17, 2023 •

edited

Loading

Rombur commented Jul 17, 2023

aprokop commented Jul 19, 2023

masterleinad commented Jul 19, 2023

masterleinad commented Jul 20, 2023

AnhBe0 commented Jul 20, 2023

Rombur Jul 20, 2023

masterleinad commented Jul 20, 2023

aprokop Jul 20, 2023

Rombur Jul 20, 2023

Rombur Aug 1, 2023

aprokop commented Aug 7, 2023

masterleinad commented Aug 7, 2023 •

edited

Loading

masterleinad commented Aug 7, 2023

masterleinad commented Aug 7, 2023

masterleinad Aug 15, 2023

masterleinad Aug 15, 2023

dalg24 Aug 21, 2023

Rombur Aug 22, 2023

dalg24 Aug 22, 2023

Rombur Aug 22, 2023

masterleinad Aug 22, 2023

dalg24 Aug 22, 2023

masterleinad Aug 22, 2023

		# FIXME: for now, skip benchmarks using Google benchmark
		# when building for Windows

Fix compile errors with MSVC(alternative to #904) #908

Fix compile errors with MSVC(alternative to #904) #908

Conversation

AnhBe0 commented Jul 14, 2023

masterleinad commented Jul 14, 2023

AnhBe0 commented Jul 14, 2023

masterleinad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masterleinad commented Jul 14, 2023

dalg24 left a comment

Choose a reason for hiding this comment

aprokop left a comment

Choose a reason for hiding this comment

Rombur commented Jul 16, 2023

aprokop commented Jul 17, 2023 • edited Loading

Rombur commented Jul 17, 2023

aprokop commented Jul 19, 2023

masterleinad commented Jul 19, 2023

masterleinad commented Jul 20, 2023

AnhBe0 commented Jul 20, 2023

Choose a reason for hiding this comment

masterleinad commented Jul 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aprokop commented Aug 7, 2023

masterleinad commented Aug 7, 2023 • edited Loading

masterleinad commented Aug 7, 2023

masterleinad commented Aug 7, 2023

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

aprokop commented Jul 17, 2023 •

edited

Loading

masterleinad commented Aug 7, 2023 •

edited

Loading