[CI] Do not enable assertions by default in configure.py script #17616
Conversation
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| with: | ||
| compiler: icx | ||
| build_configure_extra_args: -DCMAKE_C_FLAGS="/fp:precise /clang:-Wno-nonportable-include-path /clang:-Wno-cast-function-type-mismatch" -DCMAKE_CXX_FLAGS="/fp:precise /clang:-Wno-nonportable-include-path /clang:-Wno-cast-function-type-mismatch" -DCMAKE_EXE_LINKER_FLAGS=/manifest:no -DCMAKE_MODULE_LINKER_FLAGS=/manifest:no -DCMAKE_SHARED_LINKER_FLAGS=/manifest:no | ||
| build_configure_extra_args: --enable-assertions -DCMAKE_C_FLAGS="/fp:precise /clang:-Wno-nonportable-include-path /clang:-Wno-cast-function-type-mismatch" -DCMAKE_CXX_FLAGS="/fp:precise /clang:-Wno-nonportable-include-path /clang:-Wno-cast-function-type-mismatch" -DCMAKE_EXE_LINKER_FLAGS=/manifest:no -DCMAKE_MODULE_LINKER_FLAGS=/manifest:no -DCMAKE_SHARED_LINKER_FLAGS=/manifest:no |
There was a problem hiding this comment.
Note: We enable assertions when testing on windows in pre, post-commit and nightly. For more coverage, we should disable assertions in post-commit - that's what we also do for Linux.
| build_cache_root: "/__w/" | ||
| build_artifact_suffix: "default" | ||
| build_cache_suffix: "default" | ||
| build_configure_extra_args: --enable-assertions |
There was a problem hiding this comment.
how about we put this in the ci-defaults section of the configure.py script instead of yaml?
There was a problem hiding this comment.
Should this be a default in the CI? IIUC, for Linux, we enable assertions only in Nightly and pre-commit, but not during post-commit.
There was a problem hiding this comment.
ah okay i see, yeah i guess lets not change it now
| build_cache_root: "/__w/" | ||
| build_artifact_suffix: default | ||
| build_configure_extra_args: '--hip --cuda' | ||
| build_configure_extra_args: '--enable-assertions --hip --cuda' |
There was a problem hiding this comment.
@KornevNikita I'll create a followup PR to disable assertions in release branch.
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
There was a problem hiding this comment.
In release build, upstream LLVM require users to explicitly specify LLVM_ENABLE_ASSERTIONS to build with assertions.
What do mean by "LLVM release build"?
However, in configure.py script, we do the opposite: we enable assertions by default and ask users to specify --no-assertions to disable them.
This PR aligns configure.py script with the upstream.
I don't understand the motivation for that change. "aligning with the upstream" has no sense because configure.py is our own script, which has nothing to do with the upstream.
The script defaults are configured for SYCL development environment, which better enable assertions by default.
Please, provide justification for this change other than aligning with the upstream.
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
Hi Alexey, Thanks for sharing your concern. Yes,
A better question would be: why do you think we should enable assertions by default for all SYCL developers? |
Assertions is heavily used by SYCL and LLVM projects to detect errors. As I mentioned in the previous comment LLVM recommends enabling assertions for development. There are cases when assertions should be disabled to avoid an overhead added by the assertion checks, but most of SYCL developers should have assertions enabled. That's why configure.py enables assertions by default. |
In release build, upstream LLVM require users to explicitly specify
LLVM_ENABLE_ASSERTIONSto build with assertions. However, in configure.py script, we do the opposite: we enable assertions by default and ask users to specify--no-assertionsto disable them.This PR aligns configure.py script with the upstream.