Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compiling ROSCO using visual studio and intel fortran compiler #66

Closed
ghylander opened this issue Sep 14, 2021 · 39 comments
Closed

Compiling ROSCO using visual studio and intel fortran compiler #66

ghylander opened this issue Sep 14, 2021 · 39 comments

Comments

@ghylander
Copy link
Contributor

Generating the makefiles and compiling rosco using Visual Studio and Intel Fortran compiler seems to produce erroneous results

even after modifying the source code to resolve the REAL(8), and d0 explicit types, when compiling the .sln produced, the out are 3 files:
discon.dll
discon.exp
discon.lib

libdiscon.dll is nowhere to be found

@dzalkind
Copy link
Collaborator

Does discon.dll work for you when used as a controller? Perhaps that is the build name in visual studio on Windows. We aren't super familiar with that method of building ROSCO as we tend to use cmake primarily, which tends to make the compilation a little more predictable. If this compilation does work for you, we would be happy to incorporate a Visual Studio solution into this repository with your assistance.

@ghylander
Copy link
Contributor Author

For building ROSCO with visual studio i just create the makefiles using cmake and the visual studio generator ("Visual Studio 15 2017" in my case), then i open the .sln file and compile the solution. I'm trying to replicate the compilation of openfast without much success

right now what i'm having trouble with is with adding the flags (-ffree-line-length-0 -static-libgcc -static-libgfortran -static -fdefault-real-8 -fdefault-double-8 -cpp) to the visual studio compilation command, without much success

I'm currently running a baseline simulation with a known working compilation of ROSCO. After it's done i will try changing the dll and compare the results

@rafmudaf
Copy link
Collaborator

I sometimes run into problems when compiling OpenFAST via CMake and Visual Studio on Windows. So instead I've switched to using NMake Makefiles and compiling with MSVC. This looks something like this:

# Generate the NMake makefiles instead of the Visual Studio solution
cmake .. -G"NMake Makefiles"

# Use CMake's build functionality to drive NMake
cmake --build . --target all

If you do this, remember to delete your existing CMakeCache.txt. Better yet, create a new build directory and start from scratch.

right now what i'm having trouble with is with adding the flags (-ffree-line-length-0 -static-libgcc -static-libgfortran -static -fdefault-real-8 -fdefault-double-8 -cpp) to the visual studio compilation command, without much success

Where are these flags added to the project?

@ghylander
Copy link
Contributor Author

ghylander commented Sep 16, 2021

Where are these flags added to the project?

in the CMakeLists.txt:

elseif(WIN32 AND MINGW)
  # Ensure static linking to avoid requiring Fortran runtime dependencies
  set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -ffree-line-length-0 -static-libgcc -static-libgfortran -static  -fdefault-real-8 -fdefault-double-8 -cpp")

I tried removing the "AND MINGW", which passes the flags to VS. However for some reason either VS or ifort do not understand the flags. I verified that the flags are valid in the ifort version i'm using.

I'll check the MSVC workflow. In the meantime, and after much trouble, I was able to correctly compile ROSCO (I think) using cygwin and mingw64. My team however works with VS so while it's a workaround, compiling directly from VS would be best for us

@rafmudaf
Copy link
Collaborator

Hm those are GNU flags, not Intel compiler flags. A list of the current Intel compiler flags is available here.

I verified that the flags are valid in the ifort version i'm using.

It's possible somehow ifort is able to convert flags from GNU, but I'm curious how you confirmed that these work. Can you show that verification?

@ghylander
Copy link
Contributor Author

ghylander commented Sep 17, 2021

Hm those are GNU flags, not Intel compiler flags. A list of the current Intel compiler flags is available here.

I verified that the flags are valid in the ifort version i'm using.

It's possible somehow ifort is able to convert flags from GNU, but I'm curious how you confirmed that these work. Can you show that verification?

I used the ifort 17 documentation to check which flags are valid
not all the flags in gfortran are valid, but no flags are recognized. static-libgcc is a valid flag but reported as unknown (see below). I don't know if it's an issue with VS though

image

@rafmudaf
Copy link
Collaborator

It's always surprising how difficult it is to find old Intel compiler documentation. I wasn't able to, so if you could share your reference that would help.

As for static-libgcc, the current Intel compiler lists that as a linux-only flag (ref) and that makes sense for Intel on Windows. You could try sending another flag like free.

Going back to the original problem here, you've added flags that the ROSCO team intended only for MinGW. What was the original error you encountered before making changes to the CMakeLists?

@ghylander
Copy link
Contributor Author

ghylander commented Sep 20, 2021

It's always surprising how difficult it is to find old Intel compiler documentation. I wasn't able to, so if you could share your reference that would help.

The documentation is included with the installation of the compiler, under the directory:
[install-dir]\IntelSWTools\documentation_2017\en\ps2017\getstart_comp_wf.htm

As for static-libgcc, the current Intel compiler lists that as a linux-only flag (ref) and that makes sense for Intel on Windows.

That does indeed make sense, there is no explanation about that being for linux only in the documentation (as i stated above, the doc is included with the installation on a windows machine):

image

I assumed having cygwin or mingw would make the gcc libraries available for the intel compiler

You could try sending another flag like free.

Neither VS nor ifort complained about the "free" flag

Going back to the original problem here, you've added flags that the ROSCO team intended only for MinGW. What was the original error you encountered before making changes to the CMakeLists?

The only modification i made to the CMakeLists was removing "AND MINGW32". If i don't modify the CMakeLists file, VS will just compile the project without passing any flags listed in the CMakeLists to the compiler. If I remove the "AND MINGW32", VS will pass the mingw flags to ifort (and ifort will ignore them because it does not recognize them, giving the warnings in my previous comment). The optimal solution (if you are considering to add support for VS or even standalone ifort) would be to create a new if-else clause inside the WIN32 condition, similar to the Linux one. This is my current CMakeLists flags block:

image

This compiles "correctly" for both mingw and VS with ifort

@rafmudaf
Copy link
Collaborator

I agree, the control flow structure you have here provides support for Windows with Intel whereas currently there is no support.

@nikhar-abbas @dzalkind would it help if @ghylander submits a pull request with these proposed changes? Do you intend to support Windows with the Intel compiler?

FWIW the flags in the WIN32 && Intel branch would be closer to the ones for Unix with Intel. I was also surprised that CMake seems to convert the - to a / automatically for Windows, so that might allow you to simplify the flags definition further. Some flags are the same between platforms whereas others are different.

@dzalkind
Copy link
Collaborator

We'd be happy to incorporate these proposed changes. We'll try to support as many platforms as we can, but these compiler issues are still a little over my head, so I'll be referencing this issue and asking you questions if they come up in the future :-)

@ghylander
Copy link
Contributor Author

I'd say the currently standing issue would be to find ifort flags equivalent to the gfortran ones. Once this is done and the project is compiled in VS, a batch of tests can be done to validate the results are the same with both the VS and the mingw-make compiled discon.dll

@ghylander
Copy link
Contributor Author

ghylander commented Sep 21, 2021

CMake seems to convert the - to a / automatically for Windows

This is correct

An update on the ifort flags:

-cpp = -Qfpp, -Qcpp or -fpp (only -fpp worked for me), see also
-fdefault-real-8 and -fdefault-double-8 = -r8 (affects integer?, real, double and complex) or -double-size:64 and -real-size:64 (using only one of either double-size:64 or real-size:64 may be enough)
-static = -static
-ffree-line-length = -free (as far as i could tell it's not needed, ifort handles the line length if the file extension is .f90)

@rafmudaf
Copy link
Collaborator

I've taken your info above and consolidated and formatted it here for our future reference.

Flag Mapping

Here are all the flags currently in the CMake configuration:

  -DIMPLICIT_DLLEXPORT
  -ffree-line-length-0
  -fdefault-real-8
  -fdefault-double-8
  -static-libgcc
  -static-libgfortran
  -static
  -cpp

-DIMPLICIT_DLLEXPORT is specific to Unix systems so not really relevant here.

The system information in this table is only relevant to the Intel compiler. The GNU flags are used on Unix and MinGW on Windows. As such, there are no Windows-specific GNU flags here.

GNU Flag Intel Flag Windows Linux macOS
-ffree-line-length-0 -free Y Y Y
-fdefault-real-8 -real-size 64 Y Y Y
-fdefault-double-8 -double-size 64 Y Y Y
-static-libgcc -static-libgcc N Y N
-static-libgfortran N/A - - -
N/A -static Y Y Y
-cpp -fpp Y Y Y

Notes

-fdefault-real-8 and -fdefault-double-8 = -r8 (affects integer?, real, double and complex) or -double-size:64 and -real-size:64 (using only one of either double-size:64 or real-size:64 may be enough)

These settings only affect floating point numbers so thats all numbers that are not integers. Both should be set when the intention is use double precision.
Note the current CMake settings use the same precision for both real and double so that makes a double actually a real. This should be updated to -real-size 64 -double-size 128 for Intel and -fdefault-real-8 for GNU (removing -fdefault-double-8. See the GNU docs for how those flags interact.

-ffree-line-length = -free (as far as i could tell it's not needed, ifort handles the line length if the file extension is .f90)

I suggest to include this flag so that this setting is explicitly defined. Future developers may be in the habit of using .f which would unexpectedly cause different behavior.

@ghylander
Copy link
Contributor Author

Extra note:
generate the makefiles with -G "Visual Studio 15 2017 Win64"
not adding the "Win64" will create a 32bit-only project

@ghylander
Copy link
Contributor Author

I sometimes run into problems when compiling OpenFAST via CMake and Visual Studio on Windows. So instead I've switched to using NMake Makefiles and compiling with MSVC.

I tried this, but the NMake generator is pointing to the mingw64 gfortran compiler

I'm running into much trouble with compiling ROSCO:

Visual Studio and ifort: seems to be ok for now, requires validation

MinGW makefiles and cygwin64 gfortran compiler: seemed to compile correctly, but in reality seems to not function properly as it can't read the Cp_Ct_Cq file, might be a compiler problem?

MSYS generator and msys mingw64 gfortran.exe: compiles without errors but the resulting binary is much larger (2 MB) than the binary in this repo or the resulting from the VS compilation, so i'm unsure if it's correctly compiled

nmake generator: currently points to mingw64 gfortran compiler and fails to generate makefiles

@rafmudaf
Copy link
Collaborator

Extra note:
generate the makefiles with -G "Visual Studio 15 2017 Win64"
not adding the "Win64" will create a 32bit-only project

Yes, good point. This is the default behavior described in the CMake docs. Note that this is different when using later versions of Visual Studio (CMake docs VS 2019).

I tried this, but the NMake generator is pointing to the mingw64 gfortran compiler

You can directly specify the compiler you'd like to use with CMAKE_<Language>_COMPILER; i.e.

cmake .. -DCMAKE_Fortran_COMPILER=/usr/local/bin/gfortran-11 -DCMAKE_C_COMPILER=/usr/local/bin/gcc -DCMAKE_CXX_COMPILER=/usr/local/bin/g++

In case it helps, here are some good references on CMake in general and one on CMake with OpenFAST since that is configured similarly to ROSCO.

@rafmudaf
Copy link
Collaborator

What's your ultimate goal? I see that you're trying many different variations of compiler toolchains and compile settings, but it's not clear to what end.
Are you trying to establish an understanding of build systems in general or are you only trying to compile ROSCO?

@ghylander
Copy link
Contributor Author

ghylander commented Sep 24, 2021

What's your ultimate goal? I see that you're trying many different variations of compiler toolchains and compile settings, but it's not clear to what end.
Are you trying to establish an understanding of build systems in general or are you only trying to compile ROSCO?

i'm just trying to narrow down a working workflow to compile ROSCO
we are developing a custom FOWP and we would like to use ROSO as the baseline controller
as i stated earlier, my team and i are used to working with visual studio (which was my primary goal)
If compiling with VS turns out to be too complicated, we will have to use other methods, as long as they provide a correctly working binary

At the same time though i am learning compiling on the fly, previsouly i just had to follow instructions whereas now i'm the one figuring it out and writing down the instructions

You can directly specify the compiler you'd like to use with CMAKE_<Language>_COMPILER; i.e.

I will give this a shot


What kind of test can i perform to validate the correct behaviour of a compiled discon.dll? I can download the provided 5MW dll and compare it to my compilations

@rafmudaf
Copy link
Collaborator

Ok I understand. In my opinion, it's generally easier to compile Fortran code on a Unix system due to the support for open source, command-line toolchains. Basically, you can copy and paste a series of commands that use all GNU tools instead of having to set up commercial software and understand a GUI. So on Windows I've found that using the WSL is a very nice setup. You can use the familiar Windows environment for all of your data crunching (Excel or whatever else you use) and writing tasks while using the Linux system for all of the compute tasks.

Otherwise, CMake does support the Windows environment with Intel and Visual Studio, but there are many nuances. It sounds like you were able to find a working set up here, though. If so, please submit your changes as a pull request to this project for the benefit of future users.

As for tests, the ROSCO Toolbox repository contains a test suite that you can use to validate your compiled code (see the CI file). Another option is to run your controller with the OpenFAST regression test suite and verify that the results are reasonably close. What are you looking for in order to validate your binary? In other words, what are your expectations for a test and what do you want to see it compare?

@ghylander
Copy link
Contributor Author

ghylander commented Sep 30, 2021

What are you looking for in order to validate your binary? In other words, what are your expectations for a test and what do you want to see it compare?

That we are able to replicate the compilation, producing valid binaries. In short, that the compiled dll here in the repository and the one we compile ourselves without changing the source code are the same (as in, both produce the same results for a given simulation seed). Once we verify this, we would like to perhaps tune the source code.

I am familiar with WSL and use ubuntu on it on a regular basis. However, the rest of my team isn't so keen to use it

If i manage to validate the binaries obtained using cmake + ifort + VS i will be more than happy to open a pull request

@rafmudaf
Copy link
Collaborator

Ok excellent, it sounds like we are all on the same page.

Feel free to start a discussion if you'd like to talk through developing a test and evaluating the results. If you do develop a good test, we can also work together to develop a pull request to include it in the automated tests here.

@ghylander
Copy link
Contributor Author

i may have found something that may hinder this purpose, i mentioned it on a post-close comment on #68 [quote]:

it seems that if you compile a dll with a specific compiler, you will then need to execute the dll withthe same compiler runtime, otherwise this issue seems to arise.

If i downlaod the libdiscon from releases (which i presume to have been compield with gfrortran) and execute ROSCO with ifort, the controller will have trouble reading the Cp_Ct_Cq file
Similarly, if i compile ROSCO using mingw32, MSYS2 or cygwin64, and then execute the controller with ifort, again the controller will have trouble readingthe rotor performance file
However, if i compile ROSCO using ifort and then execute it with ifort too, i don't have trouble reading the performance file

@ghylander
Copy link
Contributor Author

ghylander commented Oct 13, 2021

ok, an update on this

I managed to get the run_Testing.py file to work on my machine

I ran a "binary-comp" test, using the .dl found in the latest ROSCO release and my own VS and ifort compiled binary
The results are for the most part similar once stationary regime is reached, but the controllers have different behaviour at the beginning of the simulations, as well as rarely showing different behaviour at certain points.

Here are the output pdfs. I don't know what each graph of each test represents, maybe you can help me with that:
test_outputs_GH.pdf, GH stands for GitHub

test_outputs_VS.pdf, VS stands for Visual Studio

I don't know if this difference in behaviour comes from how the variable types are declared (remember some variables have to be explicitly declared as certain types for VS+ifort), or from the missing statically linked libraries libgcc and libgfortran

@dzalkind
Copy link
Collaborator

The channels are specified in the code: https://github.com/NREL/ROSCO_toolbox/blob/95218beac3ddae18ed44925c2b31e158ce6a14b6/ROSCO_testing/ROSCO_testing.py#L509

I'm not sure why the results would be slightly different initially. We usually ignore the first 100 seconds of simulations. Is this difference important for your work?

@nikhar-abbas
Copy link
Collaborator

@ghylander just to clarify - are you compiling the v2.3.0 source code, or something slightly different? If you are compiling something like the develop or oneROSCO2 branch, I would expect some minor differences. If you are compiling the source code from v2.3.0 differences would be surprising.

The commit tagged as v2.3.0:
https://github.com/NREL/ROSCO/tree/7e100154bb8fad0fa10e7bca826d7ee1b68192b1

@ghylander
Copy link
Contributor Author

ghylander commented Oct 14, 2021

@ghylander just to clarify - are you compiling the v2.3.0 source code, or something slightly different? If you are compiling something like the develop or oneROSCO2 branch, I would expect some minor differences. If you are compiling the source code from v2.3.0 differences would be surprising.

I think I compiled the oneROSCO2 branch (as I saw that besides combining toolbox and ROSCO it also had many bugfixes and improvements over the main branch). I compiled the v2.3.0 commit (with the required modifications to compile with Visual Studio, explicitly declaring some variable types etc). As I'm typing this I am running the run_Testing.py script 'binary-comp' test with the releases downloaded .dll, the oneROSCO2 compiled .dll and the modified main branch v.2.3.0 .dll

The channels are specified in the code: https://github.com/NREL/ROSCO_toolbox/blob/95218beac3ddae18ed44925c2b31e158ce6a14b6/ROSCO_testing/ROSCO_testing.py#L509

Thanks

I'm not sure why the results would be slightly different initially. We usually ignore the first 100 seconds of simulations. Is this difference important for your work?

While it makes sense to ignore the transient stage (which can be avoided altogether by setting the appropriate initial conditions) my goal is to compile ROSCO using Visual Studio and produce a fully functional discon.dll. If the behaviours of the compiled binary found here in the GitHub (which I presume to be correctly compiled) and the one I compile myself diverge, then it must mean my compilation process is wrong

What worries me is the rarer differences found during the "steady" state. For example, in test number 10 (starting from 0), there is a very big dip around second 270 in the VS dll, when compared to the GH one:

Screenshot_1

Test number 22 (starting from 0) also exhibit some anomalies around second 580:

Screenshot_2

Test 3 (starting from 0) shows very distinct behaviour all throughout it:

Screenshot_3

@nikhar-abbas
Copy link
Collaborator

This indeed is concerningly different. Can you fork the ROSCO git repo and push the version you are working with to it? This way we can take a look at the comparison and try to hone in if this is actually just a difference in compiler methods or something is missing/problematic in the code.

Thanks,
Nikhar

@ghylander
Copy link
Contributor Author

This indeed is concerningly different. Can you fork the ROSCO git repo and push the version you are working with to it? This way we can take a look at the comparison and try to hone in if this is actually just a difference in compiler methods or something is missing/problematic in the code.

Thanks, Nikhar

I did, you can check it out in my repo, oneROSCO2 branch https://github.com/ghylander/ROSCO/tree/oneROSCO2

@nikhar-abbas
Copy link
Collaborator

nikhar-abbas commented Oct 15, 2021

This looks like it might be related to numerical issues because of the compiler changes. As was brought up in #60, this is because some of the types throughout the code are not explicitly defined. We have a pending branch that will make its way into the develop branch once oneROSCO2 is released as v2.4.0 (in the next couple of days). This should solve address some of these difficulties with compiling on windows with Intel compilers.

In the mean time, we are able to use the -r8 flag to explicitly define the precision as a compiler directive for the mac + intel case. Can that, or a similar flag, be added to your cmake declaration here: https://github.com/ghylander/ROSCO/blob/5b8ad425166fe0624ec191a8ce821be092897fb5/ROSCO/CMakeLists.txt#L24?
@rafmudaf - thoughts? I'm certainly no expert on the various compiler directives available for everything.

Here is the compare I am looking at:
oneROSCO2...ghylander:oneROSCO2
Looking at the comparer from your oneROSCO2 branch to v.2.3.0 doesn't really work because of how much things have changed.

@rafmudaf
Copy link
Collaborator

@nikhar-abbas Take a look at the notes in #66 (comment). Currently, real and double are both set to 8 bytes. I don't think this completely solves the problem, but it will help where you're seeing precision cutoffs like in #60.

@ghylander
Copy link
Contributor Author

This looks like it might be related to numerical issues because of the compiler changes. As was brought up in #60, this is because some of the types throughout the code are not explicitly defined. We have a pending branch that will make its way into the develop branch once oneROSCO2 is released as v2.4.0 (in the next couple of days). This should solve address some of these difficulties with compiling on windows with Intel compilers.

In the mean time, we are able to use the -r8 flag to explicitly define the precision as a compiler directive for the mac + intel case. Can that, or a similar flag, be added to your cmake declaration here: https://github.com/ghylander/ROSCO/blob/5b8ad425166fe0624ec191a8ce821be092897fb5/ROSCO/CMakeLists.txt#L24? @rafmudaf - thoughts? I'm certainly no expert on the various compiler directives available for everything.

Here is the compare I am looking at: oneROSCO2...ghylander:oneROSCO2 Looking at the comparer from your oneROSCO2 branch to v.2.3.0 doesn't really work because of how much things have changed.

I tried compiling the main branch of ROSCO (without the modifications to unit types etc), and the same issues appear (there are 72 total):
image

The modifications I made to unit types (based on the results of other issues related to compiling with visual studio) fix these compilation errors, but may be the cause of the inaccuracies and differences in behaviour

@nikhar-abbas
Copy link
Collaborator

@ghylander, I wouldn't expect anything to be "fixed" in the main branch yet, as we haven't merged the types update yet.

Can you add -double-size:64 to your compiler flags here:
https://github.com/ghylander/ROSCO/blob/5b8ad425166fe0624ec191a8ce821be092897fb5/ROSCO/CMakeLists.txt#L24

this will be consistent with the flags for the other build configs - where real-type=double-type=64 (yes, we are aware this is not the best practice. Will look at fixing this in a future release)

@ghylander
Copy link
Contributor Author

@ghylander, I wouldn't expect anything to be "fixed" in the main branch yet, as we haven't merged the types update yet.

Can you add -double-size:64 to your compiler flags here: https://github.com/ghylander/ROSCO/blob/5b8ad425166fe0624ec191a8ce821be092897fb5/ROSCO/CMakeLists.txt#L24

this will be consistent with the flags for the other build configs - where real-type=double-type=64 (yes, we are aware this is not the best practice. Will look at fixing this in a future release)

Surprisingly for me this worked. I thought that using real-size:64 was already enough as currently cmake treats double and reals the same way (as pointed out by Rafael above in the flag mapping comment)

can you provide a 64 bit windows binary to test my binary against?

@nikhar-abbas
Copy link
Collaborator

I'm working on getting binaries for v2.4.0 up with the release, but it might take a couple of days. In the short term, you can check using v2.3.0 as you were before.

@nikhar-abbas
Copy link
Collaborator

Hi @ghylander,
The updated binaries are now available here:
https://github.com/NREL/ROSCO/releases/tag/v2.4.0

Let us know how they compare to ROSCO when compiled with the flags we talked about in the above conversations.

Cheers,
Nikhar

@ghylander
Copy link
Contributor Author

As an update to this i might be closing this soon and makig a pull request with the cmakelists.txt changes
fdespite the differences found in the run_testing, we have been running tests with our own conditions and platform and we have not seen differences between the pre-compiled binaries and the VS code ones

@nikhar-abbas
Copy link
Collaborator

@ghylander - if you open up this PR in the next day or two, we can try to lump it together with the other open PRs for v2.4.1

@ghylander
Copy link
Contributor Author

@ghylander - if you open up this PR in the next day or two, we can try to lump it together with the other open PRs for v2.4.1

to what branch should i open the PR to? develop?

@nikhar-abbas
Copy link
Collaborator

Yes, develop. Once the fixes are in develop we can handle merging them into main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants