[CMake] Suppress -Wpass-failed warning #160472
Conversation
|
@llvm/pr-subscribers-mlir Author: Nikita Popov (nikic) Changeslibstdc++15 makes use of an unroll pragma inside std::find_if(). This produces a warning if clang fails to unroll the loop. As this pragma is outside of our control, suppress the warning. Missed transforms are not something we care about in this context. Fixes #157666. Full diff: https://github.com/llvm/llvm-project/pull/160472.diff 1 Files Affected:
diff --git a/mlir/CMakeLists.txt b/mlir/CMakeLists.txt
index 1a211f5495764..da340c4e36784 100644
--- a/mlir/CMakeLists.txt
+++ b/mlir/CMakeLists.txt
@@ -110,6 +110,12 @@ if(CMAKE_CXX_COMPILER_ID MATCHES "GNU")
endif()
endif()
+# libstdc++15 makes use of an unroll pragma inside std::find_if(). This
+# produces a warning if clang fails to unroll the loop. As this pragma is
+# outside of our control, suppress the warning.
+check_cxx_compiler_flag("-Wno-pass-failed" CXX_SUPPORTS_WNO_PASS_FAILED)
+append_if(CXX_SUPPORTS_WNO_PASS_FAILED "-Wno-pass-failed" CMAKE_CXX_FLAGS)
+
# Installing the headers and docs needs to depend on generating any public
# tablegen'd targets.
# mlir-generic-headers are dialect-independent.
|
mlir/CMakeLists.txt
Outdated
| # produces a warning if clang fails to unroll the loop. As this pragma is | ||
| # outside of our control, suppress the warning. | ||
| check_cxx_compiler_flag("-Wno-pass-failed" CXX_SUPPORTS_WNO_PASS_FAILED) | ||
| append_if(CXX_SUPPORTS_WNO_PASS_FAILED "-Wno-pass-failed" CMAKE_CXX_FLAGS) |
There was a problem hiding this comment.
It's not clear to me why is this the right fix.
This seems like a workaround for a specific version of the toolchain, but doesn't this point out to a problem with the warning itself?
Either the warning should be opt-in (that is: this warning itself is conceptually flawed somehow), or it should be able to maybe not hit on system headers (which I don't see how do implement in a robust way), or just not warn on #pragma unroll (why is it a warning an not an "optimization remark"?) ?
There was a problem hiding this comment.
Even if the warning itself can be improved (which I'm not the right person to comment on), we still need this workaround, right? I don't want to need to use Clang main to build MLIR. This is currently the only warning when building with Clang 20. It's quite annoying to have to change my LLVM_ENABLE_WERROR configuration and rebuild the world any time I need to touch MLIR for some reason.
There was a problem hiding this comment.
Sure, but it should be as narrow as possible. For example can we know in CMake what is the libstdc++ version?
Disabling gcc warnings is fairly common, but we dogfood clang warnings and I don't expect us to ever have to disable a clang warning unconditionally, instead I would think that it would get disabled for clang<20 and we also remove the warning from clang 21.
There was a problem hiding this comment.
To clarify because my previous comment was confusing: This problem still occurs with clang trunk, clang 20 is just what I use locally.
Disabling gcc warnings is fairly common, but we dogfood clang warnings and I don't expect us to ever have to disable a clang warning unconditionally
Why not? Not all warnings are interesting for all projects. Especially as we're starting from a -Wall -Wextra baseline configuration, it is expected that some warnings just aren't relevant for us. From a quick check, at least these Clang warnings are permanently disabled: -Wno-unused-parameter -Wno-long-long -Wno-noexcept-type -Wno-nested-anon-types
I would argue that this warning falls into the same category. Independently of the interaction with libstdc++ 15, this is just fundamentally not a warning we care about. It's something that LLVM runtimes might care about, but not LLVM projects. So I do think it's fine to unconditionally disable it.
There was a problem hiding this comment.
Why not? Not all warnings are interesting for all projects.
Clang warnings aren't supposed to have false positive. That is, for -Wall -Wextra, at least when I use to work on clang such warning would be removed from -Wextra and moved to -Weverything.
(also: if this shouldn't be MLIR specific, if this is a warning that the LLVM project wants to disable, I would argue to do it globally)
There was a problem hiding this comment.
I agree that this shouldn't really be MLIR specific, I've moved it into LLVM. I've also added a FIXME referencing the issue so we can reconsider disabling this warning once the specific interaction with libstdc++ is resolved.
libstdc++15 makes use of an unroll pragma inside std::find_if(). This produces a warning if clang fails to unroll the loop. As this pragma is outside of our control, suppress the warning.
nikic
force-pushed
the
mlir-pass-failed
branch
from
b42cb05 to
f203d44
Compare
nikic
changed the title
|
@joker-eph Are you fine with the updated variant? |
|
Sorry I missed the previous update, LG |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/168/builds/16549 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/95/builds/18655 Here is the relevant piece of the build log for the reference |
libstdc++15 makes use of an unroll pragma inside std::find_if(). This produces a warning if clang fails to unroll the loop. As this pragma is outside of our control, suppress the warning. Missed transforms are not something we care about in this context.
Related to #157666.