[Eigen] c2228 .size must have class/struct/union on Windows

[lukeiwanski]

Following conversation on https://github.com/lukeiwanski/tensorflow/issues/202#issuecomment-372546220 an issue has been found in Eigen for Windows platform.

In Meta.h on lines 333-334:

template Index size(const T& x) { return x.size(); }

Produces error c2228 with the message "left of '.size' must have class/struct/union".

[lukeiwanski]

@welnaseth could you provide full error? We need to find out what T is :)

[welnaseth]

Sure thing, I'll get some details on my steps to repro and what Visual Studio spits out as the error after work today.

[welnaseth]

So, I can't get the error to repro anymore. I had deleted the files generated by cmake and re-ran it to get the flags I had set, and now the Experimental target built with no compiler errors (there were 2 before). IDK what happened, but i guess I must have botched something during the setup. I'll be away from my computer until monday, so I'll try to build it again then just to confirm. For now, here are my steps to build for windows:

1. Download and install latest ComputeCPP on Windows
2. Clone this branch of Eigen for Opencl
3. Replace /{location of cloned repo}/cmake/FindComputeCpp.cmake with the version found here to add support for finding ComputeCPP on windows.
4. Generate MSVC files for Visual Studio 15 Win 64 using CMake with EIGEN_TEST_SYCL=true and COMPUTECPP_PACKAGE_ROOT_DIR={ComputeCPP install location}.
5. Open Generated project in Visual Studio 2017 and try to build the Experimental target (to run the tests)

Currently the first test (rand) fails, but I've got an early flight tomorrow, so I can't wait until all the tests finish. How about we keep this open until Monday when I can confirm the issues is gone or if someone else confirms no build errors with my process before then. Monday I should be able to get a rundown of what tests fail and why.

[ghost]

@welnaseth Seems your link is broken, it should be this one: https://bitbucket.org/mehdi_goli/opencl/commits/branch/Eigen-Optimised-Tensor-Vector-Contraction

[ghost]

I follow your steps, and I got the ComputeCpp found message:

-- ComputeCpp package - Found
-- compute++ - Found
-- computecpp_info - Found
-- ComputeCpp runtime (Release): C:/Program Files/Codeplay/ComputeCpp/lib/ComputeCpp_vs2015.lib - Found
-- ComputeCpp runtime  (Debug) : C:/Program Files/Codeplay/ComputeCpp/lib/ComputeCpp_vs2015_d.lib - Found
-- ComputeCpp includes - Found
-- Package version - CE 0.6.1
-- compute++ flags - -O2 -mllvm -inline-threshold=1000 -sycl -emit-llvm -intelspirmetadata

While the final result still reports SYCL: OFF. I don't know if that will cause my test incorrect.

-- ************************************************************
-- ***    Eigen's unit tests configuration summary          ***
-- ************************************************************
--
-- Build type:        Release
-- Build site:        karl-1231v3
-- Build string:      win7-19.0.24215.1-32bit
-- Enabled backends:
-- Disabled backends: Cholmod,  UmfPack,  SuperLU,  PaStiX,  METIS,  SPQR,  Qt4
support,  Boost.Multiprecision,  Xsmm,  GoogleHash,  Adolc,  MPFR C++,  fftw,  O
penGL,
-- Default order:     Column-major
-- Maximal matrix/vector size: 320
-- SSE2:              Using architecture defaults
-- SSE3:              Using architecture defaults
-- SSSE3:             Using architecture defaults
-- SSE4.1:            Using architecture defaults
-- SSE4.2:            Using architecture defaults
-- AVX:               Using architecture defaults
-- FMA:               Using architecture defaults
-- AVX512:            Using architecture defaults
-- Altivec:           Using architecture defaults
-- VSX:               Using architecture defaults
-- ARM NEON:          Using architecture defaults
-- ARMv8 NEON:        Using architecture defaults
-- S390X ZVECTOR:     Using architecture defaults
-- C++11:             OFF
-- SYCL:              OFF
-- CUDA:              OFF
--
CXX:               C:/Program Files (x86)/Microsoft Visual Studio 14.0/VC/bin/cl
.exe
 CXX_FLAGS:         /DWIN32 /D_WINDOWS /W4 /GR /EHsc /EHsc /wd4127 /wd4505 /wd47
14 /D_CRT_SECURE_NO_WARNINGS /D_SCL_SECURE_NO_WARNINGS
 Sparse lib flags:

-- ************************************************************
--
-- Configured Eigen 3.3.90
--
-- To build/run the unit tests, read this page:
--   http://eigen.tuxfamily.org/index.php?title=Tests
--
-- Configuring done

Also, I got a 777 succeeded result:

========== Build: 777 succeeded, 0 failed, 0 up-to-date, 8 skipped ==========

build.log

[lukeiwanski]

@kcauchy you need to add EIGEN_TEST_CXX11 and EIGEN_TEST_SYCL to cmake. (SYCL requires C++11 features)

[ghost]

@lukeiwanski Thank you.

By compiling command:

C:\Users\karl\Desktop\opencl\build>cmake -DEIGEN_TEST_SYCL=1 -DEIGEN_TEST_CXX11=
1 -DCOMPUTE_CPP_PAKCAGE_ROOT_DIR="C:\Program Files\Codeplay\ComputeCpp" ..

I still got the error below: (many copies)

CMake Error at cmake/EigenTesting.cmake:131 (add_sycl_to_target):
  add_sycl_to_target Function invoked with incorrect arguments for function
  named: add_sycl_to_target
Call Stack (most recent call first):
  cmake/EigenTesting.cmake:307 (ei_add_test_internal_sycl)
  unsupported/test/CMakeLists.txt:170 (ei_add_test_sycl)

I've already replaced the FindComputeCpp.cmake with this one.

I'm using the revision 10900 at branch Eigen-Optimised-Tensor-Vector-Contraction.

[DuncanMcBain]

There were some interface changes between the FindComputeCpp module in the SDK and here. I can try to provide a sort of hacky workaround on Monday, but we should update the Eigen version to the newer SDK revisions.

[welnaseth]

I am having the same errors as @kcauchy, I'll look into it more later tonight, but figured I'd chime in confirming the errors. Should we close this bug and move discussion to a bug with a more appropriate title such as "Eigen CMake changes for Opencl on Windows" or something of the sort?

[DuncanMcBain]

Ah yes, the hacky workaround I promised... Really at this stage the best option would be to update the Eigen CMake properly. I'm not sure if you're familiar with CMake, but broadly the issue is that the function "add_sycl_to_target" changed the order of its arguments upstream, and this change hasn't yet been reflected in Eigen (if it ain't broke, don't fix it, I suppose). Changing all the calling code to match the new function signature should fix this particular problem (I've not had time to make the "hacky" solution so far this week, I'm afraid).

[welnaseth]

Okay, so can I confirm where the change actually is? I found this commit that takes away two of the arguments. However, this change to the function isn't reflected in Eigen, or the computecpp-sdk FindComputeCpp.cmake. Adding the two arguments back to the call in EigenTesting.cmake seems to allow cmake to configure correctly.

Has there been a change to the dev version of FindComputeCpp.cmake that hasn't been reflected in the public SDK?

[DuncanMcBain]

The version in the SDK is the way, the truth and the light :p I'm not exactly sure of the context in that commit but it appears to me like it simply removes the arguments rather swapping them. When I said "to match the new function signature", I meant to match the one that can be found in the SDK.

As you can tell, this process could stand to be improved.

If you try changing the interface Eigen is using to match the SDK FindComputeCpp.cmake and it works, let us know! :)

[welnaseth]

Maybe @lukeiwanski could shed some light on the commit :P I did run it last night with the change and cmake was able finish and generate the project. I didn't notice some errors that must not be fatal errors, so I'll rerun it again and post what I get as output as it generates.

After that I did try building the experimental build target, and boy did it not go well. Claimed it had 1300 compiler errors and some of the tests were failing. I would post more details, but bit was late and I had fallen asleep while it was compiling and was too sleepy to investigate when I woke up. However as seen earlier in this post, sometimes my computer likes to give me errors that don't actually exist. @kcauchy, could you try as well? Of you just reverse the commit I linked, cmake should build.

Tonight I'll rebuild the experimental target and see if I can get more details on the errors. I also get my specs too, just to confirm my issues wouldn't be due to my hardware/drivers.

[lukeiwanski]

hey @welnaseth!

Maybe @lukeiwanski could shed some light on the commit

The intention of mentioned commit or rather this pull request was to introduce the ComputeCpp driver. Instead of two explicit compiler passes (one for the device and one for the host) you can call ComputeCpp driver that will do everything in the background for you. That makes stuff like cross-compilation or CMake integration much easier.

That said, ComputeCpp driver has been tested only for Linux (so far) and you exposed valid problem that needs to be addressed!

Hope that helps

[welnaseth]

Sorry that I haven't posted an update recently, I've been busy with other things. I'll try to check on my compiler errors soon and see what I can glean from them.

[lukeiwanski]

@welnaseth Thales is working on fixes for Windows in Eigen ( https://bitbucket.org/mehdi_goli/opencl/pull-requests/38/changes-in-eigen-to-build-sycl-tests-on/diff#comment-61508492 ). Hopefully we will get that in soon. I will ping you once that happens.

[Rbiessy]

The fix is in. You can use ComputeCpp driver by defining -DCOMPUTECPP_USE_COMPILER_DRIVER to the cmake. Note that the Eigen fork has moved to: https://bitbucket.org/codeplaysoftware/eigen/src/default/