allscale / allscale_compiler

The AllScale Compiler providing a high-performance, portable implementation of the AllScale API
5 stars 1 forks source link

Compiler flags for integration testing and allscalecc are broken #4

Open sithhell opened 6 years ago

sithhell commented 6 years ago

I want to make the compiler flags for integration testing and running allscalecc directly consistent.

So far, the places I found where the flags are configured in code/allscale_integration_test_config.in and code/compiler/src/backend/allscale_backend.cpp. I am not sure where and when the compile flags are set. and at which places they need to change.

Essentially, what I am trying to accomplish is the following:

diff --git a/code/compiler/src/backend/allscale_backend.cpp b/code/compiler/src/backend/allscale_backend.cpp
index bfc80db..5eeda24 100644
--- a/code/compiler/src/backend/allscale_backend.cpp
+++ b/code/compiler/src/backend/allscale_backend.cpp
@@ -105,9 +105,19 @@ namespace backend {
                compiler.addFlag("-Wl,--no-as-needed");

                // add libraries
-               compiler.addExternalLibrary(ALLSCALE_RUNTIME_LIBRARY_DIR, "allscale");
+               compiler.addExternalLibrary(ALLSCALE_RUNTIME_LIBRARY_DIR, "hpx_allscale");
+#if defined(ALLSCALE_RUNTIME_DEBUG)
+               compiler.addFlag("-D_DEBUG");
+               compiler.addFlag("-DDEBUG");
+               compiler.addExternalLibrary(HPX_LIBRARY_DIR, "hpx_initd");
+               compiler.addExternalLibrary(HPX_LIBRARY_DIR, "hpxd");
+#else
+               compiler.addFlag("-DNDEBUG");
+               compiler.addFlag("-DHPX_DISABLE_ASSERTS");
+               compiler.addFlag("-DBOOST_DISABLE_ASSERTS");
                compiler.addExternalLibrary(HPX_LIBRARY_DIR, "hpx_init");
                compiler.addExternalLibrary(HPX_LIBRARY_DIR, "hpx");
+#endif
                compiler.addExternalLibrary(BOOST_LIBRARY_DIR, "boost_chrono");
                compiler.addExternalLibrary(BOOST_LIBRARY_DIR, "boost_date_time");
                compiler.addExternalLibrary(BOOST_LIBRARY_DIR, "boost_filesystem");

with ALLSCALE_RUNTIME_DEBUG being set when the CMAKE_BUILD_TYPE is Debug.

Any hints or suggestions?

HerbertJordan commented 6 years ago

Not a fan of the macro. What you want is to that in case the build is a debug build the first set of flags is used, for release builds the other set of flags? The selection should be based on e.g. the optimization level passed to the compiler, at runtime. However, those flags are used in case the compiler is also invoking the backend compiler, like it is the case when it is used e.g. in iPIC3d.

For the integration tests: here, to support manual debugging, the allscalecc compiler is only producing target code, which is then directly compiled to a binary using g++. I would suggest a fixed set of flags, in this case, preferably debug flags, and they have to be added here: https://github.com/allscale/allscale_compiler/blob/master/test/config#L5

Is the list of flags you mentioned complete? Then I would add it and get it done.

HerbertJordan commented 6 years ago

Actually, after investigating: the compiler has to pass the -DXXX flags through as specified by the user. The decision whether to add or remove those flags should remain with the user.

For the pilots, thus the corresponding cmake scripts have to set those flags.

sithhell commented 6 years ago

It boils down to the build system being completely broken for this usecase. I can not easily mix and match build types for the compiler and runtime. As the runtime developer, for example, I want to have both, a debug build (where this might mean different flavors of different debug options) and a release build. Right now, there is no way to change that.

On possible solution would be to rely on hpxcc (which should pass on the correct flags) instead of whatever other backend compiler was used. This way, it should be trivial to mix and match different allscale runtime builds by just pointing PATH to the correct (tm) hpxcc script.

I have no idea where to turn the knobs here.

HerbertJordan commented 6 years ago

It shouldn't be a surprise that a build and test infrastructure for the compiler has not been designed with the runtime developers' needs in mind. After all, it is to aid the compiler development. We had to build them for our needs, you may customize it to your own needs or ask for additional features.

As a quick step, if you want to run integration tests manually against different builts of the runtime, you can use the integration test tool to get the commands for building and running test cases and adjust paths manually. You may also create an issue requesting a feature for adding support to the infrastructure for using external runtime system builds.

For now, commit 876b9abfc1b65477cc0c02ec234c4a14c7829691 adds the -DHPX_DISABLE_ASSERTS and the -DBOOST_DISABLE_ASSERTS flag in any case, since I guess we should be able to assume that a) our input codes do not contain HPX nor BOOST codes (the latter may be relaxed later on) and b) we should also be able to assume that HPX is stable enough to be used. If you want to make those optional, let me know. Otherwise I'd consider the topic of this issue done..

sithhell commented 6 years ago

The problem arises when you want to actual produce meaningful binaries. For example when doing performance tests. The changes I suggested above would actually help the compiler developers as well. Consider it as a feature request.

HerbertJordan commented 6 years ago

I thought that was answered: The integration test driver is not intended for that purpose, and will never be.

If you want to compile binaries with 'allscalecc' for performance tests, you do the same as for gcc: you invoke it with the -O3 -DNDEBUG flags. Those are then forwarded to the backend compiler when building the target code (this is not running through the integration test system). The HPX_DISABLE_ASSERTS and BOOST equaivalent is now on by default. And the compiler uses a release build of the runtime for creating the binary anyway (soon).

What is missing?

sithhell commented 6 years ago

I think the easiest solution for this problem is to allow the user to set the location of hpx and the runtime. at the same time, extract the correct compile flags from hpx's cmake infrastructure instead of hardcoding it. This would allow for quick'n'easy to start builds, with whatever defaults you think make sense as well as for more complex work flows and use cases.

sithhell commented 6 years ago

I'd still prefer a complete dynamic solution (through hpxcc as mentioned earlier) since the allscale compiler build itself doesn't depend on the runtime per se. I'm fine with either solution. The benefit: compile once, use with multiple builds of the runtime.

HerbertJordan commented 6 years ago

The AllScale Runtime System has been integrated into the compiler build based on the information and resource that have been available to us when we implemented it. If you desire an alternative variant, work with the build infrastructure guys by explaining your idea with a little more detail than a few phrases and check whether it fits the use cases and usability objectives of the compiler and integrate it together with them if you (plural) think it is worth the effort.

As said, the current tools have been developed for integration testing the compiler. We can not read minds and/or predict the future to foresee uses cases of development work we are not involved in.

sithhell commented 6 years ago

It all boils down to what the produced allscalecc should accomplish: Is it a complete wrapper to hide the include paths, library paths and other flags for the underlying system? Specifically, the API and the Runtime (Including its dependency)

I personally think this should be the goal. If you disagree, that's fine, but then we need to work on documenting and properly. One way forward would be to use the pkg-config facilities provided by HPX to solve the runtime build flag issues as documented here: https://stellar-group.github.io/hpx/docs/html/hpx/manual/build_system/using_hpx/pkgconfig.html

This would allow us, depending on where PKG_CONFIG_PATH points to, to select the right configurations. This would blend in nicely with the already existing integration testing infrastructure, as a default is easily provided.

There is also a wrapper script which is generated under ${HPX_ROOT}/bin/hpxcc which wraps this logic. As such, I propose that the compiler should use this as the backend compiler.

This is what's behind the "phrases" above.

sithhell commented 6 years ago

The biggest advantage is, that the allscale compiler doesn't have to guess the flags, and potential changes are automatically propagated.

sithhell commented 6 years ago

Could this be easily fixed by setting the INSIEME_BACKEND_CXX_COMPILER cmake variable to hpxcc and omit the HPX specific build flags in the integration test driver and backend (In addition to set the PATH env variable to ${HPX_ROOT}/bin in the integration test driver)?

HerbertJordan commented 6 years ago

I actually don't know the hpxcc and how it is used. If it is a g++ replacement that adds definitions and libraries to link, this should work. I guess the only way to know is to try it.

If we would switch to this, how do we switch between runtime builds?

sithhell commented 6 years ago

hpxcc is a wrapper around the compiler that invokes the compiler HPX was built with the appropriate linker, include and other compile flags. All other flags are forwarded to that compiler.

Switching between runtime builds is indeed not covered by this scenario. you probably would require an additional switch for allscalecc, something like --allscale_runtime=/path/to/runtime/build.

How about a solution, where the allscale runtime build provides the flags for the compiler? An obvious solution would be to invoke pkg-config (does not properly work on windows). The runtime build, on the other hand, could provide the config files, in a similar way than the integration testing is currently getting it's information. Essentially, we bring the responsibilty for the correct build settings for the runtime to the runtime itself. This would cover the basic flags, include directories for HPX and the runtime, as well as the needed libraries and potential additional flags (ALLSCALE_WITH_HPX as well, for example).

Would this work?

I'd need to know the format etc. that you expect, and would be able to implmenent it accordingly.

HerbertJordan commented 6 years ago

I have to forward this to @W4RH4WK who will be back in 2h or so.