Skip to content

Set default CMAKE_BUILD_TYPE to Release and implement CMAKE_BUILD_TYPE FastBuild#474

Closed
marbre wants to merge 6 commits intoiree-org:masterfrom
marbre:cmake_build_type
Closed

Set default CMAKE_BUILD_TYPE to Release and implement CMAKE_BUILD_TYPE FastBuild#474
marbre wants to merge 6 commits intoiree-org:masterfrom
marbre:cmake_build_type

Conversation

@marbre
Copy link
Copy Markdown
Member

@marbre marbre commented Jan 14, 2020

  • Sets the default CMAKE_BUILD_TYPE to Release
  • Implements a custom build type FastBuild
    • If the CMAKE_BUILD_TYPE is set to FastBuild, for LLVM the CMAKE_BUILD_TYPE is set to Relase

LLVM only allows DEBUG|RELEASE|RELWITHDEBINFO|MINSIZEREL.
Hence, if the build type is set to fastbuild, we set the
CMAKE_BUILD_TYPE to Release for LLVM.
@GMNGeoffrey
Copy link
Copy Markdown
Contributor

So it looks like this increases the build time by another third (40 minutes, vs 30 minutes for the last run on master: https://github.com/google/iree/commit/fea7914fdd77e93976c055c366cd9d8373fae1d0/checks?check_suite_id=399128460). That surprises me. I would have expected it to be faster than debug. Should we reconsider making release the default? Maybe we should default to fastbuild

@marbre
Copy link
Copy Markdown
Member Author

marbre commented Jan 14, 2020

Well regarding the default in the CMakeLists.txt, I think Release is the option to go. Anyway, fell free to pass -DCMAKE_BUILD_TYPE=FastBuild within the build pipeline. At least on my build machine #475 seems to be no issue with CMake.

@GMNGeoffrey
Copy link
Copy Markdown
Contributor

Why do you say Release is better? You're probably the main one building with cmake, so I'm fine deferring to your judgement on that, but curious about your reasoning.

Just doing a rough and unscientific test using time on my workstation using the same script as the CI

cmake \
  -DCMAKE_BUILD_TYPE=${BUILD_TYPE} \
  -DIREE_BUILD_COMPILER=ON \
  -DIREE_BUILD_TESTS=ON \
  -DIREE_BUILD_SAMPLES=OFF \
  -DIREE_BUILD_DEBUGGER=OFF \
  ..

With BUILD_TYPE=
Release: 19m58s
FastBuild: 17m19s
Debug: 15m22s

So for strange reasons, Debug is actually even faster than FastBuild and it seems like things are about twice as fast on my workstation as on the GitHub runner. I guess if we start actually running tests, this balance would change quite a bit, but it seems like while we're just trying to get as much building as possible that the fastest build is the most desirable? It also calls into question somewhat the utility of fastbuild at all.

@marbre
Copy link
Copy Markdown
Member Author

marbre commented Jan 14, 2020

Well, normally I build software with optimizations enabled, since I am not scared about compile time but rather about execution time. So I assume that Release would be the suitable default for a user.

Regarding compile time, setting FastBuild, LLVM is actually build with CMAKE_BUILD_TYPE=Release, so with optimizations. With Debug set, IREE as well as LLVM are build without optimizations. I think that is the reason for FastBuild beeing build slower than Debug.

@GMNGeoffrey
Copy link
Copy Markdown
Contributor

Fair enough. For now, since we're not actually running anything based on the build, let's at least default the CI to something that builds quickly. Can you change that as part of this PR? (build_tools/scripts/cmake_build.sh)

Maybe for FastBuild we should build LLVM with Debug if that's faster? I'm testing that out now to see how it looks.

@marbre
Copy link
Copy Markdown
Member Author

marbre commented Jan 14, 2020

Sure, I can adjust cmake_build.sh :) However, the question remains if we only want to keep 694d681 and set the CI to Debug in addition, but skip the remaining commits for FastBuild. If it just blows up the CMakeLists.txt without any benefit I think we should rather drop those changes.

@GMNGeoffrey
Copy link
Copy Markdown
Contributor

Well I forgot to actually run it with time 🤦‍♂️, so all I've got is the timestamps I have printed as part of my prompt. Running again, but it does look like FastBuild with LLVM set to Debug is slightly faster than everything Debug (14m). I'm fine doing that for now. Or if you'd prefer, we can just drop FastBuild and stick with default Release but CI using Debug

@GMNGeoffrey
Copy link
Copy Markdown
Contributor

GMNGeoffrey commented Jan 14, 2020

Yeah running with time, FastBuild setting LLVM to Debug is the fastest by a about 30 seconds (14m50s). Not a huge difference though.

@marbre
Copy link
Copy Markdown
Member Author

marbre commented Jan 14, 2020

Okay, done. The CI should build with Debug, which is LLVM's default for now as well. Thus I think it might be okay to keep the FastBuild setting for now. We can drop it later anyway if there isn't much benefit, but at least it makes the CMake and Bazel builds more comparable.
If @stellaraccident or @benvanik do not have other suggestions, this can be merged. An alternative would be to only cherry-pick 694d681 and 4c8285b.

Comment thread build_tools/scripts/cmake_build.sh Outdated
rm -rf build/
mkdir build && cd build
"$CMAKE_BIN" -DIREE_BUILD_COMPILER=ON -DIREE_BUILD_TESTS=ON -DIREE_BUILD_SAMPLES=OFF -DIREE_BUILD_DEBUGGER=OFF ..
"$CMAKE_BIN" -DCMAKE_BUILD_TYPE=Debug -DIREE_BUILD_COMPILER=ON -DIREE_BUILD_TESTS=ON -DIREE_BUILD_SAMPLES=OFF -DIREE_BUILD_DEBUGGER=OFF ..
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was saying I think this should be FastBuild

Suggested change
"$CMAKE_BIN" -DCMAKE_BUILD_TYPE=Debug -DIREE_BUILD_COMPILER=ON -DIREE_BUILD_TESTS=ON -DIREE_BUILD_SAMPLES=OFF -DIREE_BUILD_DEBUGGER=OFF ..
"$CMAKE_BIN" -DCMAKE_BUILD_TYPE=FastBuild -DIREE_BUILD_COMPILER=ON -DIREE_BUILD_TESTS=ON -DIREE_BUILD_SAMPLES=OFF -DIREE_BUILD_DEBUGGER=OFF ..

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're absolutely right. Sorry for that, as an excuse, it's quite late here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I could have picked that directly here on GitHub. Wasn't aware of this feature. Next time :)

@GMNGeoffrey
Copy link
Copy Markdown
Contributor

Build times on the CI look better now, thanks!

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

Successfully merging this pull request may close these issues.

3 participants