[clang] 76cad51 - replace clang LLVM_ENABLE_PLUGINS -> CLANG_PLUGIN_SUPPORT in tests
Author: Jameson Nash Date: 2022-02-09T17:31:34-05:00 New Revision: 76cad51ba700233d6e3492eddcbb466b6adbc2eb URL: https://github.com/llvm/llvm-project/commit/76cad51ba700233d6e3492eddcbb466b6adbc2eb DIFF: https://github.com/llvm/llvm-project/commit/76cad51ba700233d6e3492eddcbb466b6adbc2eb.diff LOG: replace clang LLVM_ENABLE_PLUGINS -> CLANG_PLUGIN_SUPPORT in tests Ensure CLANG_PLUGIN_SUPPORT is compatible with llvm_add_library. Fixes an issue noted in D00. Differential Revision: https://reviews.llvm.org/D119199 Added: Modified: clang-tools-extra/test/CMakeLists.txt clang-tools-extra/test/lit.site.cfg.py.in clang/CMakeLists.txt clang/examples/AnnotateFunctions/CMakeLists.txt clang/examples/Attribute/CMakeLists.txt clang/examples/CallSuperAttribute/CMakeLists.txt clang/examples/PluginsOrder/CMakeLists.txt clang/examples/PrintFunctionNames/CMakeLists.txt clang/lib/Analysis/plugins/CMakeLists.txt clang/test/CMakeLists.txt clang/test/lit.site.cfg.py.in clang/tools/driver/CMakeLists.txt Removed: diff --git a/clang-tools-extra/test/CMakeLists.txt b/clang-tools-extra/test/CMakeLists.txt index 9321457ae1a39..170e5f8bd197d 100644 --- a/clang-tools-extra/test/CMakeLists.txt +++ b/clang-tools-extra/test/CMakeLists.txt @@ -17,7 +17,7 @@ string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} CLANG_TOOLS_DIR ${LLVM_RUN llvm_canonicalize_cmake_booleans( CLANG_TIDY_ENABLE_STATIC_ANALYZER - LLVM_ENABLE_PLUGINS + CLANG_PLUGIN_SUPPORT LLVM_INSTALL_TOOLCHAIN_ONLY ) @@ -87,10 +87,19 @@ if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY) PLUGIN_TOOL clang-tidy DEPENDS clang-tidy-headers) + if(CLANG_BUILT_STANDALONE) +# LLVMHello library is needed below +if (EXISTS ${LLVM_MAIN_SRC_DIR}/lib/Transforms/Hello + AND NOT TARGET LLVMHello) + add_subdirectory(${LLVM_MAIN_SRC_DIR}/lib/Transforms/Hello +lib/Transforms/Hello) +endif() + endif() + if(TARGET CTTestTidyModule) list(APPEND CLANG_TOOLS_TEST_DEPS CTTestTidyModule LLVMHello) target_include_directories(CTTestTidyModule PUBLIC BEFORE "${CLANG_TOOLS_SOURCE_DIR}") - if(LLVM_ENABLE_PLUGINS AND (WIN32 OR CYGWIN)) + if(CLANG_PLUGIN_SUPPORT AND (WIN32 OR CYGWIN)) set(LLVM_LINK_COMPONENTS Support ) diff --git a/clang-tools-extra/test/lit.site.cfg.py.in b/clang-tools-extra/test/lit.site.cfg.py.in index e7db0e2ef2cb8..d30e6664816b7 100644 --- a/clang-tools-extra/test/lit.site.cfg.py.in +++ b/clang-tools-extra/test/lit.site.cfg.py.in @@ -12,7 +12,7 @@ config.clang_libs_dir = "@SHLIBDIR@" config.python_executable = "@Python3_EXECUTABLE@" config.target_triple = "@TARGET_TRIPLE@" config.clang_tidy_staticanalyzer = @CLANG_TIDY_ENABLE_STATIC_ANALYZER@ -config.has_plugins = @LLVM_ENABLE_PLUGINS@ & ~@LLVM_INSTALL_TOOLCHAIN_ONLY@ +config.has_plugins = @CLANG_PLUGIN_SUPPORT@ & ~@LLVM_INSTALL_TOOLCHAIN_ONLY@ # Support substitution of the tools and libs dirs with user parameters. This is # used when we can't determine the tool dir at configuration time. diff --git a/clang/CMakeLists.txt b/clang/CMakeLists.txt index 1cc9bacf4e5eb..4225c028e1794 100644 --- a/clang/CMakeLists.txt +++ b/clang/CMakeLists.txt @@ -476,6 +476,10 @@ add_definitions( -D_GNU_SOURCE ) option(CLANG_BUILD_TOOLS "Build the Clang tools. If OFF, just generate build targets." ON) +CMAKE_DEPENDENT_OPTION(CLANG_PLUGIN_SUPPORT + "Build clang with plugin support" ON + "LLVM_ENABLE_PLUGINS OR LLVM_EXPORT_SYMBOLS_FOR_PLUGINS" OFF) + option(CLANG_ENABLE_ARCMT "Build ARCMT." ON) option(CLANG_ENABLE_STATIC_ANALYZER "Include static analyzer in clang binary." ON) diff --git a/clang/examples/AnnotateFunctions/CMakeLists.txt b/clang/examples/AnnotateFunctions/CMakeLists.txt index e9850b64f08d7..e6541f7cc62a6 100644 --- a/clang/examples/AnnotateFunctions/CMakeLists.txt +++ b/clang/examples/AnnotateFunctions/CMakeLists.txt @@ -1,6 +1,6 @@ add_llvm_library(AnnotateFunctions MODULE AnnotateFunctions.cpp PLUGIN_TOOL clang) -if(LLVM_ENABLE_PLUGINS AND (WIN32 OR CYGWIN)) +if(CLANG_PLUGIN_SUPPORT AND (WIN32 OR CYGWIN)) set(LLVM_LINK_COMPONENTS Support ) diff --git a/clang/examples/Attribute/CMakeLists.txt b/clang/examples/Attribute/CMakeLists.txt index 42f04f5039bc7..5392ac0df1703 100644 --- a/clang/examples/Attribute/CMakeLists.txt +++ b/clang/examples/Attribute/CMakeLists.txt @@ -1,6 +1,6 @@ add_llvm_library(Attribute MODULE Attribute.cpp PLUGIN_TOOL clang) -if(LLVM_ENABLE_PLUGINS AND (WIN32 OR CYGWIN)) +if(CLANG_PLUGIN_SUPPORT AND (WIN32 OR CYGWIN)) target_link_libraries(Attribute PRIVATE clangAST clangBasic diff --git a/clang/examples/CallSuperAttribute/CMakeLists.txt b/clang/examples/CallSuperAttribute/CMakeLists.txt index 922f0cfa797a8..f4284adf289e3 100644 --- a/clang/examples/CallSuperAttribute/CMakeLists.txt +++ b/clang/exa
[clang] 9d59cfc - clang-analyzer plugins require LLVM_ENABLE_PLUGINS also
Author: Jameson Nash Date: 2022-02-16T11:59:09-05:00 New Revision: 9d59cfc67eadbe4b4088d70f8bbc61c96568d2f1 URL: https://github.com/llvm/llvm-project/commit/9d59cfc67eadbe4b4088d70f8bbc61c96568d2f1 DIFF: https://github.com/llvm/llvm-project/commit/9d59cfc67eadbe4b4088d70f8bbc61c96568d2f1.diff LOG: clang-analyzer plugins require LLVM_ENABLE_PLUGINS also The clang-analyzer plugins are not linked to a particular tool, so they can only be compiled if plugins are broadly supported. We could opt instead to decide whether to link them to specifically against clang or with undefined symbols, depending on the value of LLVM_ENABLE_PLUGINS, but we do not currently expect there to be a use case for that rather niche configuration. Differential Revision: https://reviews.llvm.org/D119591 Added: Modified: clang/CMakeLists.txt clang/examples/AnnotateFunctions/CMakeLists.txt clang/examples/Attribute/CMakeLists.txt clang/examples/CMakeLists.txt clang/examples/CallSuperAttribute/CMakeLists.txt clang/examples/PluginsOrder/CMakeLists.txt clang/examples/PrintFunctionNames/CMakeLists.txt clang/lib/Analysis/plugins/CMakeLists.txt clang/test/CMakeLists.txt Removed: diff --git a/clang/CMakeLists.txt b/clang/CMakeLists.txt index 4225c028e1794..937a8467df1d7 100644 --- a/clang/CMakeLists.txt +++ b/clang/CMakeLists.txt @@ -476,9 +476,14 @@ add_definitions( -D_GNU_SOURCE ) option(CLANG_BUILD_TOOLS "Build the Clang tools. If OFF, just generate build targets." ON) +if(LLVM_ENABLE_PLUGINS OR LLVM_EXPORT_SYMBOLS_FOR_PLUGINS) + set(HAVE_CLANG_PLUGIN_SUPPORT ON) +else() + set(HAVE_CLANG_PLUGIN_SUPPORT OFF) +endif() CMAKE_DEPENDENT_OPTION(CLANG_PLUGIN_SUPPORT "Build clang with plugin support" ON - "LLVM_ENABLE_PLUGINS OR LLVM_EXPORT_SYMBOLS_FOR_PLUGINS" OFF) + "HAVE_CLANG_PLUGIN_SUPPORT" OFF) option(CLANG_ENABLE_ARCMT "Build ARCMT." ON) option(CLANG_ENABLE_STATIC_ANALYZER diff --git a/clang/examples/AnnotateFunctions/CMakeLists.txt b/clang/examples/AnnotateFunctions/CMakeLists.txt index e6541f7cc62a6..827ff79fcc410 100644 --- a/clang/examples/AnnotateFunctions/CMakeLists.txt +++ b/clang/examples/AnnotateFunctions/CMakeLists.txt @@ -1,6 +1,6 @@ add_llvm_library(AnnotateFunctions MODULE AnnotateFunctions.cpp PLUGIN_TOOL clang) -if(CLANG_PLUGIN_SUPPORT AND (WIN32 OR CYGWIN)) +if(WIN32 OR CYGWIN) set(LLVM_LINK_COMPONENTS Support ) diff --git a/clang/examples/Attribute/CMakeLists.txt b/clang/examples/Attribute/CMakeLists.txt index 5392ac0df1703..770fa28364b77 100644 --- a/clang/examples/Attribute/CMakeLists.txt +++ b/clang/examples/Attribute/CMakeLists.txt @@ -1,6 +1,6 @@ add_llvm_library(Attribute MODULE Attribute.cpp PLUGIN_TOOL clang) -if(CLANG_PLUGIN_SUPPORT AND (WIN32 OR CYGWIN)) +if(WIN32 OR CYGWIN) target_link_libraries(Attribute PRIVATE clangAST clangBasic diff --git a/clang/examples/CMakeLists.txt b/clang/examples/CMakeLists.txt index 4a677933bb54f..8be327bcdbb9d 100644 --- a/clang/examples/CMakeLists.txt +++ b/clang/examples/CMakeLists.txt @@ -3,8 +3,10 @@ if(NOT CLANG_BUILD_EXAMPLES) set(EXCLUDE_FROM_ALL ON) endif() -add_subdirectory(PrintFunctionNames) -add_subdirectory(AnnotateFunctions) -add_subdirectory(Attribute) -add_subdirectory(CallSuperAttribute) -add_subdirectory(PluginsOrder) +if(CLANG_PLUGIN_SUPPORT) + add_subdirectory(PrintFunctionNames) + add_subdirectory(AnnotateFunctions) + add_subdirectory(Attribute) + add_subdirectory(CallSuperAttribute) + add_subdirectory(PluginsOrder) +endif() diff --git a/clang/examples/CallSuperAttribute/CMakeLists.txt b/clang/examples/CallSuperAttribute/CMakeLists.txt index f4284adf289e3..dc832327c775d 100644 --- a/clang/examples/CallSuperAttribute/CMakeLists.txt +++ b/clang/examples/CallSuperAttribute/CMakeLists.txt @@ -1,6 +1,6 @@ add_llvm_library(CallSuperAttr MODULE CallSuperAttrInfo.cpp PLUGIN_TOOL clang) -if(CLANG_PLUGIN_SUPPORT AND (WIN32 OR CYGWIN)) +if(WIN32 OR CYGWIN) set(LLVM_LINK_COMPONENTS Support ) diff --git a/clang/examples/PluginsOrder/CMakeLists.txt b/clang/examples/PluginsOrder/CMakeLists.txt index 612587a6d2fe3..c22b0081ad686 100644 --- a/clang/examples/PluginsOrder/CMakeLists.txt +++ b/clang/examples/PluginsOrder/CMakeLists.txt @@ -1,6 +1,6 @@ add_llvm_library(PluginsOrder MODULE PluginsOrder.cpp PLUGIN_TOOL clang) -if(CLANG_PLUGIN_SUPPORT AND (WIN32 OR CYGWIN)) +if(WIN32 OR CYGWIN) set(LLVM_LINK_COMPONENTS Support ) diff --git a/clang/examples/PrintFunctionNames/CMakeLists.txt b/clang/examples/PrintFunctionNames/CMakeLists.txt index 67f1b16744eaf..28da363729f3a 100644 --- a/clang/examples/PrintFunctionNames/CMakeLists.txt +++ b/clang/examples/PrintFunctionNames/CMakeLists.txt @@ -11,7 +11,7 @@ endif() add_llvm_library(PrintFunctionNames MODULE PrintFunctionNames.cpp PLUGIN_TOOL clang) -if(CLANG_PLUGIN_SUPPORT AND (WIN32 OR CYG
[clang-tools-extra] [clang-tidy] Enable plugin tests with LLVM_INSTALL_TOOLCHAIN_ONLY (PR #90370)
vtjnash wrote: I think out of tree builds of clang-tidy (back in the svn days, when people did partial checkouts of individual projects) probably needed this to be able to correctly correctly find the right headers. Maybe standalone builds aren't permitted anymore? I think I mentioned this a bit in earlier revisions of that PR https://reviews.llvm.org/D00#3284178 https://github.com/llvm/llvm-project/pull/90370 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] 3689272 - enable plugins for clang-tidy
Author: Jameson Nash Date: 2022-01-29T14:21:19-05:00 New Revision: 36892727e4f19a60778e371d78f8fb09d8122c85 URL: https://github.com/llvm/llvm-project/commit/36892727e4f19a60778e371d78f8fb09d8122c85 DIFF: https://github.com/llvm/llvm-project/commit/36892727e4f19a60778e371d78f8fb09d8122c85.diff LOG: enable plugins for clang-tidy Fixes #32739 Differential Revision: https://reviews.llvm.org/D00 Added: clang-tools-extra/test/clang-tidy/CTTestTidyModule.cpp Modified: clang-tools-extra/clang-tidy/tool/CMakeLists.txt clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/Contributing.rst clang-tools-extra/docs/clang-tidy/index.rst clang-tools-extra/test/CMakeLists.txt clang-tools-extra/test/lit.cfg.py clang-tools-extra/test/lit.site.cfg.py.in Removed: diff --git a/clang-tools-extra/clang-tidy/tool/CMakeLists.txt b/clang-tools-extra/clang-tidy/tool/CMakeLists.txt index 4b8c93801501a..3ce552872015e 100644 --- a/clang-tools-extra/clang-tidy/tool/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/tool/CMakeLists.txt @@ -29,11 +29,17 @@ clang_target_link_libraries(clangTidyMain clangToolingCore ) +# Support plugins. +if(CLANG_PLUGIN_SUPPORT) + set(support_plugins SUPPORT_PLUGINS) +endif() + add_clang_tool(clang-tidy ClangTidyToolMain.cpp - ) -add_dependencies(clang-tidy + + DEPENDS clang-resource-headers + ${support_plugins} ) clang_target_link_libraries(clang-tidy PRIVATE @@ -50,6 +56,9 @@ target_link_libraries(clang-tidy ${ALL_CLANG_TIDY_CHECKS} ) +if(CLANG_PLUGIN_SUPPORT) + export_executable_symbols_for_plugins(clang-tidy) +endif() install(PROGRAMS clang-tidy- diff .py DESTINATION "${CMAKE_INSTALL_DATADIR}/clang" diff --git a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp index 6147d90eb10b9..1b0010bdd62a2 100644 --- a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp +++ b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp @@ -20,6 +20,7 @@ #include "../GlobList.h" #include "clang/Tooling/CommonOptionsParser.h" #include "llvm/Support/InitLLVM.h" +#include "llvm/Support/PluginLoader.h" #include "llvm/Support/Process.h" #include "llvm/Support/Signals.h" #include "llvm/Support/TargetSelect.h" @@ -386,6 +387,11 @@ getVfsFromFile(const std::string &OverlayFile, int clangTidyMain(int argc, const char **argv) { llvm::InitLLVM X(argc, argv); + + // Enable help for -load option, if plugins are enabled. + if (cl::Option *LoadOpt = cl::getRegisteredOptions().lookup("load")) +LoadOpt->addCategory(ClangTidyCategory); + llvm::Expected OptionsParser = CommonOptionsParser::create(argc, argv, ClangTidyCategory, cl::ZeroOrMore); diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 0e9491eab9f6a..eceed8749c950 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -86,6 +86,8 @@ Improvements to clang-tidy - Eliminated false positives for `cppcoreguidelines-macro-usage` by restricting the warning about using constants to only macros that expand to literals. +- Added support for external plugin checks with `-load`. + New checks ^^ diff --git a/clang-tools-extra/docs/clang-tidy/Contributing.rst b/clang-tools-extra/docs/clang-tidy/Contributing.rst index b9eb0e7627cc1..b1771574950a8 100644 --- a/clang-tools-extra/docs/clang-tidy/Contributing.rst +++ b/clang-tools-extra/docs/clang-tidy/Contributing.rst @@ -634,6 +634,26 @@ This keeps the test directory from getting cluttered. .. _FileCheck: https://llvm.org/docs/CommandGuide/FileCheck.html .. _test/clang-tidy/google-readability-casting.cpp: https://reviews.llvm.org/ diff usion/L/browse/clang-tools-extra/trunk/test/clang-tidy/google-readability-casting.cpp +Out-of-tree check plugins +- + +Developing an out-of-tree check as a plugin largely follows the steps +outlined above. The plugin is a shared library whose code lives outside +the clang-tidy build system. Build and link this shared library against +LLVM as done for other kinds of Clang plugins. + +The plugin can be loaded by passing `-load` to `clang-tidy` in addition to the +names of the checks to enable. + +.. code-block:: console + + $ clang-tidy --checks=-*,my-explicit-constructor -list-checks -load myplugin.so + +There is no expectations regarding ABI and API stability, so the plugin must be +compiled against the version of clang-tidy that will be loading the plugin. + +The plugins can use threads, TLS, or any other facilities available to in-tree +code which is accessible from the external headers. Running clang-tidy on LLVM -- diff --git a/clang-tools-extra/docs/clang-tidy/index.
[clang-tools-extra] 84f137a - Reland "enable plugins for clang-tidy"
Author: Jameson Nash Date: 2022-02-01T17:37:24-05:00 New Revision: 84f137a590e7de25c4105303e5938c40566c2dfb URL: https://github.com/llvm/llvm-project/commit/84f137a590e7de25c4105303e5938c40566c2dfb DIFF: https://github.com/llvm/llvm-project/commit/84f137a590e7de25c4105303e5938c40566c2dfb.diff LOG: Reland "enable plugins for clang-tidy" This reverts commit ab3b89855c5318f0009e1f016ffe5b1483507fd0 but disables the new test if the user has disabled support for building it. Added: clang-tools-extra/test/clang-tidy/CTTestTidyModule.cpp Modified: clang-tools-extra/clang-tidy/tool/CMakeLists.txt clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/Contributing.rst clang-tools-extra/docs/clang-tidy/index.rst clang-tools-extra/test/CMakeLists.txt clang-tools-extra/test/lit.cfg.py clang-tools-extra/test/lit.site.cfg.py.in Removed: diff --git a/clang-tools-extra/clang-tidy/tool/CMakeLists.txt b/clang-tools-extra/clang-tidy/tool/CMakeLists.txt index 4b8c93801501a..3ce552872015e 100644 --- a/clang-tools-extra/clang-tidy/tool/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/tool/CMakeLists.txt @@ -29,11 +29,17 @@ clang_target_link_libraries(clangTidyMain clangToolingCore ) +# Support plugins. +if(CLANG_PLUGIN_SUPPORT) + set(support_plugins SUPPORT_PLUGINS) +endif() + add_clang_tool(clang-tidy ClangTidyToolMain.cpp - ) -add_dependencies(clang-tidy + + DEPENDS clang-resource-headers + ${support_plugins} ) clang_target_link_libraries(clang-tidy PRIVATE @@ -50,6 +56,9 @@ target_link_libraries(clang-tidy ${ALL_CLANG_TIDY_CHECKS} ) +if(CLANG_PLUGIN_SUPPORT) + export_executable_symbols_for_plugins(clang-tidy) +endif() install(PROGRAMS clang-tidy- diff .py DESTINATION "${CMAKE_INSTALL_DATADIR}/clang" diff --git a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp index 6147d90eb10b9..1b0010bdd62a2 100644 --- a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp +++ b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp @@ -20,6 +20,7 @@ #include "../GlobList.h" #include "clang/Tooling/CommonOptionsParser.h" #include "llvm/Support/InitLLVM.h" +#include "llvm/Support/PluginLoader.h" #include "llvm/Support/Process.h" #include "llvm/Support/Signals.h" #include "llvm/Support/TargetSelect.h" @@ -386,6 +387,11 @@ getVfsFromFile(const std::string &OverlayFile, int clangTidyMain(int argc, const char **argv) { llvm::InitLLVM X(argc, argv); + + // Enable help for -load option, if plugins are enabled. + if (cl::Option *LoadOpt = cl::getRegisteredOptions().lookup("load")) +LoadOpt->addCategory(ClangTidyCategory); + llvm::Expected OptionsParser = CommonOptionsParser::create(argc, argv, ClangTidyCategory, cl::ZeroOrMore); diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index d1f7f6bdf05dc..7f4f814221f73 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -207,6 +207,8 @@ Improvements to clang-tidy - Added support for `NOLINTBEGIN` ... `NOLINTEND` comments to suppress Clang-Tidy warnings over multiple lines. +- Added support for external plugin checks with `-load`. + New checks ^^ diff --git a/clang-tools-extra/docs/clang-tidy/Contributing.rst b/clang-tools-extra/docs/clang-tidy/Contributing.rst index b9eb0e7627cc1..b1771574950a8 100644 --- a/clang-tools-extra/docs/clang-tidy/Contributing.rst +++ b/clang-tools-extra/docs/clang-tidy/Contributing.rst @@ -634,6 +634,26 @@ This keeps the test directory from getting cluttered. .. _FileCheck: https://llvm.org/docs/CommandGuide/FileCheck.html .. _test/clang-tidy/google-readability-casting.cpp: https://reviews.llvm.org/ diff usion/L/browse/clang-tools-extra/trunk/test/clang-tidy/google-readability-casting.cpp +Out-of-tree check plugins +- + +Developing an out-of-tree check as a plugin largely follows the steps +outlined above. The plugin is a shared library whose code lives outside +the clang-tidy build system. Build and link this shared library against +LLVM as done for other kinds of Clang plugins. + +The plugin can be loaded by passing `-load` to `clang-tidy` in addition to the +names of the checks to enable. + +.. code-block:: console + + $ clang-tidy --checks=-*,my-explicit-constructor -list-checks -load myplugin.so + +There is no expectations regarding ABI and API stability, so the plugin must be +compiled against the version of clang-tidy that will be loading the plugin. + +The plugins can use threads, TLS, or any other facilities available to in-tree +code which is accessible from the external headers. Running clang-tidy on LLVM -- diff --git a/clan
[clang] [analyzer] Do not destruct fields of unions (PR #122330)
https://github.com/vtjnash updated https://github.com/llvm/llvm-project/pull/122330 >From 829e1c89ce869f782cb802a1d618003770c0d074 Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Thu, 9 Jan 2025 17:10:08 + Subject: [PATCH 1/3] [Sema] do not destruct fields of unions The C++ standard prohibits this implicit destructor call, leading to incorrect reports from clang-analyzer. This causes projects that use std::option (including llvm) to fail the cplusplus.NewDelete test incorrectly when run through the analyzer. Fixes #119415 --- clang/lib/Analysis/CFG.cpp| 2 ++ .../test/Analysis/NewDelete-checker-test.cpp | 28 +++ clang/test/Analysis/dtor-array.cpp| 24 3 files changed, 54 insertions(+) diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp index 304bbb2b422c61..3e144395cffc6f 100644 --- a/clang/lib/Analysis/CFG.cpp +++ b/clang/lib/Analysis/CFG.cpp @@ -2041,6 +2041,8 @@ void CFGBuilder::addImplicitDtorsForDestructor(const CXXDestructorDecl *DD) { } // First destroy member objects. + if (RD->isUnion()) +return; for (auto *FI : RD->fields()) { // Check for constant size array. Set type to array element type. QualType QT = FI->getType(); diff --git a/clang/test/Analysis/NewDelete-checker-test.cpp b/clang/test/Analysis/NewDelete-checker-test.cpp index 21b4cf817b5df6..806edd47840fc1 100644 --- a/clang/test/Analysis/NewDelete-checker-test.cpp +++ b/clang/test/Analysis/NewDelete-checker-test.cpp @@ -441,3 +441,31 @@ void testLeakBecauseNTTPIsNotDeallocation() { void* p = ::operator new(10); deallocate_via_nttp(p); } // leak-warning{{Potential leak of memory pointed to by 'p'}} + +namespace optional_union { + template + class unique_ptr { +T *q; + public: +unique_ptr() : q(new T) {} +~unique_ptr() { + delete q; +} + }; + + union custom_union_t { +unique_ptr present; +char notpresent; +custom_union_t() : present(unique_ptr()) {} +~custom_union_t() {}; + }; + + void testUnionCorrect() { +custom_union_t a; +a.present.~unique_ptr(); + } + + void testUnionLeak() { +custom_union_t a; + } // leak-warning{{Potential leak of memory pointed to by 'a.present.q'}} +} diff --git a/clang/test/Analysis/dtor-array.cpp b/clang/test/Analysis/dtor-array.cpp index 84a34af9225169..1bbe55c09ee7e2 100644 --- a/clang/test/Analysis/dtor-array.cpp +++ b/clang/test/Analysis/dtor-array.cpp @@ -377,3 +377,27 @@ void directUnknownSymbol() { } } + +void testUnionDtor() { + static int unionDtorCalled; + InlineDtor::cnt = 0; + InlineDtor::dtorCalled = 0; + unionDtorCalled = 0; + { + union UnionDtor { + InlineDtor kind1; + char kind2; + ~UnionDtor() { unionDtorCalled++; } + }; + UnionDtor u1{.kind1{}}; + UnionDtor u2{.kind2{}}; + auto u3 = new UnionDtor{.kind1{}}; + auto u4 = new UnionDtor{.kind2{}}; + delete u3; + delete u4; + } + + clang_analyzer_eval(unionDtorCalled == 4); // expected-warning {{TRUE}} + clang_analyzer_eval(InlineDtor::dtorCalled != 4); // expected-warning {{TRUE}} + clang_analyzer_eval(InlineDtor::dtorCalled == 0); // expected-warning {{TRUE}} +} >From 032315608bca99b3da9a74cb1d7f497bce66b47d Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Wed, 29 Jan 2025 19:24:39 + Subject: [PATCH 2/3] fixup! [Sema] do not destruct fields of unions --- clang/docs/ReleaseNotes.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 7fafe2807bd388..1c23b48d8a1df2 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -103,6 +103,8 @@ Attribute Changes in Clang Improvements to Clang's diagnostics --- +- Fixed a bug where Clang's Analysis did not correctly model the destructor behavior of ``union`` members (#GH119415). + Improvements to Clang's time-trace -- >From d8a6ab76553d301c63e0c3bc60870655cd428ea1 Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Mon, 3 Feb 2025 11:31:16 -0500 Subject: [PATCH 3/3] Update clang/test/Analysis/dtor-array.cpp Co-authored-by: Balazs Benics --- clang/test/Analysis/dtor-array.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/clang/test/Analysis/dtor-array.cpp b/clang/test/Analysis/dtor-array.cpp index 1bbe55c09ee7e2..942cba4a2e046d 100644 --- a/clang/test/Analysis/dtor-array.cpp +++ b/clang/test/Analysis/dtor-array.cpp @@ -398,6 +398,5 @@ void testUnionDtor() { } clang_analyzer_eval(unionDtorCalled == 4); // expected-warning {{TRUE}} - clang_analyzer_eval(InlineDtor::dtorCalled != 4); // expected-warning {{TRUE}} clang_analyzer_eval(InlineDtor::dtorCalled == 0); // expected-warning {{TRUE}} } ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Do not destruct fields of unions (PR #122330)
@@ -377,3 +377,27 @@ void directUnknownSymbol() { } } + +void testUnionDtor() { + static int unionDtorCalled; + InlineDtor::cnt = 0; + InlineDtor::dtorCalled = 0; + unionDtorCalled = 0; + { + union UnionDtor { + InlineDtor kind1; + char kind2; + ~UnionDtor() { unionDtorCalled++; } + }; + UnionDtor u1{.kind1{}}; + UnionDtor u2{.kind2{}}; + auto u3 = new UnionDtor{.kind1{}}; + auto u4 = new UnionDtor{.kind2{}}; + delete u3; + delete u4; + } + + clang_analyzer_eval(unionDtorCalled == 4); // expected-warning {{TRUE}} + clang_analyzer_eval(InlineDtor::dtorCalled != 4); // expected-warning {{TRUE}} vtjnash wrote: Sure, makes sense. I had left it in as a hint of what the expected bad result would look like in case it breaks, but not necessary https://github.com/llvm/llvm-project/pull/122330 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Do not destruct fields of unions (PR #122330)
https://github.com/vtjnash updated https://github.com/llvm/llvm-project/pull/122330 >From 0293a835a395c2e5a54efd16b70a38e73f116c59 Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Thu, 9 Jan 2025 17:10:08 + Subject: [PATCH 1/3] [Sema] do not destruct fields of unions The C++ standard prohibits this implicit destructor call, leading to incorrect reports from clang-analyzer. This causes projects that use std::option (including llvm) to fail the cplusplus.NewDelete test incorrectly when run through the analyzer. Fixes #119415 --- clang/lib/Analysis/CFG.cpp| 2 ++ .../test/Analysis/NewDelete-checker-test.cpp | 28 +++ clang/test/Analysis/dtor-array.cpp| 24 3 files changed, 54 insertions(+) diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp index 304bbb2b422c61d..3e144395cffc6ff 100644 --- a/clang/lib/Analysis/CFG.cpp +++ b/clang/lib/Analysis/CFG.cpp @@ -2041,6 +2041,8 @@ void CFGBuilder::addImplicitDtorsForDestructor(const CXXDestructorDecl *DD) { } // First destroy member objects. + if (RD->isUnion()) +return; for (auto *FI : RD->fields()) { // Check for constant size array. Set type to array element type. QualType QT = FI->getType(); diff --git a/clang/test/Analysis/NewDelete-checker-test.cpp b/clang/test/Analysis/NewDelete-checker-test.cpp index 21b4cf817b5df6b..806edd47840fc11 100644 --- a/clang/test/Analysis/NewDelete-checker-test.cpp +++ b/clang/test/Analysis/NewDelete-checker-test.cpp @@ -441,3 +441,31 @@ void testLeakBecauseNTTPIsNotDeallocation() { void* p = ::operator new(10); deallocate_via_nttp(p); } // leak-warning{{Potential leak of memory pointed to by 'p'}} + +namespace optional_union { + template + class unique_ptr { +T *q; + public: +unique_ptr() : q(new T) {} +~unique_ptr() { + delete q; +} + }; + + union custom_union_t { +unique_ptr present; +char notpresent; +custom_union_t() : present(unique_ptr()) {} +~custom_union_t() {}; + }; + + void testUnionCorrect() { +custom_union_t a; +a.present.~unique_ptr(); + } + + void testUnionLeak() { +custom_union_t a; + } // leak-warning{{Potential leak of memory pointed to by 'a.present.q'}} +} diff --git a/clang/test/Analysis/dtor-array.cpp b/clang/test/Analysis/dtor-array.cpp index 84a34af92251692..1bbe55c09ee7e27 100644 --- a/clang/test/Analysis/dtor-array.cpp +++ b/clang/test/Analysis/dtor-array.cpp @@ -377,3 +377,27 @@ void directUnknownSymbol() { } } + +void testUnionDtor() { + static int unionDtorCalled; + InlineDtor::cnt = 0; + InlineDtor::dtorCalled = 0; + unionDtorCalled = 0; + { + union UnionDtor { + InlineDtor kind1; + char kind2; + ~UnionDtor() { unionDtorCalled++; } + }; + UnionDtor u1{.kind1{}}; + UnionDtor u2{.kind2{}}; + auto u3 = new UnionDtor{.kind1{}}; + auto u4 = new UnionDtor{.kind2{}}; + delete u3; + delete u4; + } + + clang_analyzer_eval(unionDtorCalled == 4); // expected-warning {{TRUE}} + clang_analyzer_eval(InlineDtor::dtorCalled != 4); // expected-warning {{TRUE}} + clang_analyzer_eval(InlineDtor::dtorCalled == 0); // expected-warning {{TRUE}} +} >From ec5ae1bd8976ea52f34500aabf73d6e47c4430b5 Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Wed, 29 Jan 2025 19:24:39 + Subject: [PATCH 2/3] fixup! [Sema] do not destruct fields of unions --- clang/docs/ReleaseNotes.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 18ecf1efa13a164..8973c49c051a3dd 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -121,6 +121,7 @@ Improvements to Clang's diagnostics - The ``-Wunique-object-duplication`` warning has been added to warn about objects which are supposed to only exist once per program, but may get duplicated when built into a shared library. +- Fixed a bug where Clang's Analysis did not correctly model the destructor behavior of ``union`` members (#GH119415). Improvements to Clang's time-trace -- >From 5c581bb54baa154a00a347b2df66d2ffa0990e18 Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Tue, 4 Feb 2025 21:00:21 + Subject: [PATCH 3/3] fixup! [Sema] do not destruct fields of unions --- .../test/Analysis/NewDelete-checker-test.cpp | 2 +- clang/test/Analysis/dtor-array.cpp| 24 clang/test/Analysis/dtor-union.cpp| 38 +++ 3 files changed, 39 insertions(+), 25 deletions(-) create mode 100644 clang/test/Analysis/dtor-union.cpp diff --git a/clang/test/Analysis/NewDelete-checker-test.cpp b/clang/test/Analysis/NewDelete-checker-test.cpp index 806edd47840fc11..06754f669b1e61b 100644 --- a/clang/test/Analysis/NewDelete-checker-test.cpp +++ b/clang/test/Analysis/NewDelete-checker-test.cpp @@ -457,7 +457,7 @@ namespace optional_union { unique_ptr prese
[clang] [analyzer] Do not destruct fields of unions (PR #122330)
@@ -441,3 +441,31 @@ void testLeakBecauseNTTPIsNotDeallocation() { void* p = ::operator new(10); deallocate_via_nttp(p); } // leak-warning{{Potential leak of memory pointed to by 'p'}} + +namespace optional_union { + template + class unique_ptr { +T *q; + public: +unique_ptr() : q(new T) {} +~unique_ptr() { + delete q; +} + }; + + union custom_union_t { +unique_ptr present; +char notpresent; +custom_union_t() : present(unique_ptr()) {} +~custom_union_t() {}; + }; + + void testUnionCorrect() { +custom_union_t a; +a.present.~unique_ptr(); + } + + void testUnionLeak() { vtjnash wrote: If I don't declare a dtor, it seems to be a C++ syntax error, so I cannot test that, but I will try to fix the other nits https://github.com/llvm/llvm-project/pull/122330 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Sema] do not destruct fields of unions (PR #122330)
https://github.com/vtjnash updated https://github.com/llvm/llvm-project/pull/122330 >From 829e1c89ce869f782cb802a1d618003770c0d074 Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Thu, 9 Jan 2025 17:10:08 + Subject: [PATCH 1/2] [Sema] do not destruct fields of unions The C++ standard prohibits this implicit destructor call, leading to incorrect reports from clang-analyzer. This causes projects that use std::option (including llvm) to fail the cplusplus.NewDelete test incorrectly when run through the analyzer. Fixes #119415 --- clang/lib/Analysis/CFG.cpp| 2 ++ .../test/Analysis/NewDelete-checker-test.cpp | 28 +++ clang/test/Analysis/dtor-array.cpp| 24 3 files changed, 54 insertions(+) diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp index 304bbb2b422c61..3e144395cffc6f 100644 --- a/clang/lib/Analysis/CFG.cpp +++ b/clang/lib/Analysis/CFG.cpp @@ -2041,6 +2041,8 @@ void CFGBuilder::addImplicitDtorsForDestructor(const CXXDestructorDecl *DD) { } // First destroy member objects. + if (RD->isUnion()) +return; for (auto *FI : RD->fields()) { // Check for constant size array. Set type to array element type. QualType QT = FI->getType(); diff --git a/clang/test/Analysis/NewDelete-checker-test.cpp b/clang/test/Analysis/NewDelete-checker-test.cpp index 21b4cf817b5df6..806edd47840fc1 100644 --- a/clang/test/Analysis/NewDelete-checker-test.cpp +++ b/clang/test/Analysis/NewDelete-checker-test.cpp @@ -441,3 +441,31 @@ void testLeakBecauseNTTPIsNotDeallocation() { void* p = ::operator new(10); deallocate_via_nttp(p); } // leak-warning{{Potential leak of memory pointed to by 'p'}} + +namespace optional_union { + template + class unique_ptr { +T *q; + public: +unique_ptr() : q(new T) {} +~unique_ptr() { + delete q; +} + }; + + union custom_union_t { +unique_ptr present; +char notpresent; +custom_union_t() : present(unique_ptr()) {} +~custom_union_t() {}; + }; + + void testUnionCorrect() { +custom_union_t a; +a.present.~unique_ptr(); + } + + void testUnionLeak() { +custom_union_t a; + } // leak-warning{{Potential leak of memory pointed to by 'a.present.q'}} +} diff --git a/clang/test/Analysis/dtor-array.cpp b/clang/test/Analysis/dtor-array.cpp index 84a34af9225169..1bbe55c09ee7e2 100644 --- a/clang/test/Analysis/dtor-array.cpp +++ b/clang/test/Analysis/dtor-array.cpp @@ -377,3 +377,27 @@ void directUnknownSymbol() { } } + +void testUnionDtor() { + static int unionDtorCalled; + InlineDtor::cnt = 0; + InlineDtor::dtorCalled = 0; + unionDtorCalled = 0; + { + union UnionDtor { + InlineDtor kind1; + char kind2; + ~UnionDtor() { unionDtorCalled++; } + }; + UnionDtor u1{.kind1{}}; + UnionDtor u2{.kind2{}}; + auto u3 = new UnionDtor{.kind1{}}; + auto u4 = new UnionDtor{.kind2{}}; + delete u3; + delete u4; + } + + clang_analyzer_eval(unionDtorCalled == 4); // expected-warning {{TRUE}} + clang_analyzer_eval(InlineDtor::dtorCalled != 4); // expected-warning {{TRUE}} + clang_analyzer_eval(InlineDtor::dtorCalled == 0); // expected-warning {{TRUE}} +} >From 032315608bca99b3da9a74cb1d7f497bce66b47d Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Wed, 29 Jan 2025 19:24:39 + Subject: [PATCH 2/2] fixup! [Sema] do not destruct fields of unions --- clang/docs/ReleaseNotes.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 7fafe2807bd388..1c23b48d8a1df2 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -103,6 +103,8 @@ Attribute Changes in Clang Improvements to Clang's diagnostics --- +- Fixed a bug where Clang's Analysis did not correctly model the destructor behavior of ``union`` members (#GH119415). + Improvements to Clang's time-trace -- ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Sema] do not destruct fields of unions (PR #122330)
vtjnash wrote: This roughly corresponds to 921bd20dddf5080cdb36f39c0162eb63b2d5325e in Sema, I think (@zygoloid). Maybe @tkremenek or @yronglin has done something in this part of the code recently too as reviewers and to merge this? https://github.com/llvm/llvm-project/pull/122330 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Sema] do not destruct fields of unions (PR #122330)
https://github.com/vtjnash created https://github.com/llvm/llvm-project/pull/122330 The C++ standard prohibits this implicit destructor call, leading to incorrect reports from clang-analyzer. This causes projects that use std::option (including llvm) to fail the cplusplus.NewDelete test incorrectly when run through the analyzer. Fixes #119415 >From 533e475ffa57da335a2cce387f08c83f442fd16b Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Thu, 9 Jan 2025 17:10:08 + Subject: [PATCH] [Sema] do not destruct fields of unions The C++ standard prohibits this implicit destructor call, leading to incorrect reports from clang-analyzer. This causes projects that use std::option (including llvm) to fail the cplusplus.NewDelete test incorrectly when run through the analyzer. Fixes #119415 --- clang/lib/Analysis/CFG.cpp| 2 ++ .../test/Analysis/NewDelete-checker-test.cpp | 28 +++ clang/test/Analysis/dtor-array.cpp| 24 3 files changed, 54 insertions(+) diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp index 304bbb2b422c61..3e144395cffc6f 100644 --- a/clang/lib/Analysis/CFG.cpp +++ b/clang/lib/Analysis/CFG.cpp @@ -2041,6 +2041,8 @@ void CFGBuilder::addImplicitDtorsForDestructor(const CXXDestructorDecl *DD) { } // First destroy member objects. + if (RD->isUnion()) +return; for (auto *FI : RD->fields()) { // Check for constant size array. Set type to array element type. QualType QT = FI->getType(); diff --git a/clang/test/Analysis/NewDelete-checker-test.cpp b/clang/test/Analysis/NewDelete-checker-test.cpp index 21b4cf817b5df6..806edd47840fc1 100644 --- a/clang/test/Analysis/NewDelete-checker-test.cpp +++ b/clang/test/Analysis/NewDelete-checker-test.cpp @@ -441,3 +441,31 @@ void testLeakBecauseNTTPIsNotDeallocation() { void* p = ::operator new(10); deallocate_via_nttp(p); } // leak-warning{{Potential leak of memory pointed to by 'p'}} + +namespace optional_union { + template + class unique_ptr { +T *q; + public: +unique_ptr() : q(new T) {} +~unique_ptr() { + delete q; +} + }; + + union custom_union_t { +unique_ptr present; +char notpresent; +custom_union_t() : present(unique_ptr()) {} +~custom_union_t() {}; + }; + + void testUnionCorrect() { +custom_union_t a; +a.present.~unique_ptr(); + } + + void testUnionLeak() { +custom_union_t a; + } // leak-warning{{Potential leak of memory pointed to by 'a.present.q'}} +} diff --git a/clang/test/Analysis/dtor-array.cpp b/clang/test/Analysis/dtor-array.cpp index 84a34af9225169..1bbe55c09ee7e2 100644 --- a/clang/test/Analysis/dtor-array.cpp +++ b/clang/test/Analysis/dtor-array.cpp @@ -377,3 +377,27 @@ void directUnknownSymbol() { } } + +void testUnionDtor() { + static int unionDtorCalled; + InlineDtor::cnt = 0; + InlineDtor::dtorCalled = 0; + unionDtorCalled = 0; + { + union UnionDtor { + InlineDtor kind1; + char kind2; + ~UnionDtor() { unionDtorCalled++; } + }; + UnionDtor u1{.kind1{}}; + UnionDtor u2{.kind2{}}; + auto u3 = new UnionDtor{.kind1{}}; + auto u4 = new UnionDtor{.kind2{}}; + delete u3; + delete u4; + } + + clang_analyzer_eval(unionDtorCalled == 4); // expected-warning {{TRUE}} + clang_analyzer_eval(InlineDtor::dtorCalled != 4); // expected-warning {{TRUE}} + clang_analyzer_eval(InlineDtor::dtorCalled == 0); // expected-warning {{TRUE}} +} ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [InstCombine] remove dead loads, such as memcpy from undef (PR #143958)
https://github.com/vtjnash created https://github.com/llvm/llvm-project/pull/143958 @nikic The implements our discussion in https://github.com/llvm/llvm-project/pull/143782#discussion_r2142720376, extending `isAllocSiteRemovable` to be able to check if the ModRef info indicates the alloca is only Ref or only Mod, and be able to remove it accordingly. It seemed that there were a surprising number of benchmarks with this pattern which weren't getting optimized previously (due to MemorySSA walk limits). There were somewhat more existing tests than I'd like to have modified which were simply doing exactly this pattern. This PR was going to be an experiment with using Claude to write code, but I didn't like its implementation (I asked it to do the wrong approach), so the code is mine. Though Claude did contribute the tests and found an important typo that I'd made. >From 0d309ccbde665a033976a9eaa9f84c284765825c Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Thu, 12 Jun 2025 18:48:00 + Subject: [PATCH 1/2] [instcombine] remove dead loads, such as memcpy from undef --- .../InstCombine/InstructionCombining.cpp | 66 +-- .../Transforms/InstCombine/malloc-free.ll | 6 +- 2 files changed, 60 insertions(+), 12 deletions(-) diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp index dc2a8cb0115e7..aa7119b005013 100644 --- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp +++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp @@ -3217,10 +3217,12 @@ static bool isRemovableWrite(CallBase &CB, Value *UsedV, static bool isAllocSiteRemovable(Instruction *AI, SmallVectorImpl &Users, - const TargetLibraryInfo &TLI) { + const TargetLibraryInfo &TLI, + bool KnowInit) { SmallVector Worklist; const std::optional Family = getAllocationFamily(AI, &TLI); Worklist.push_back(AI); + ModRefInfo Access = KnowInit ? ModRefInfo::NoModRef : ModRefInfo::Mod; do { Instruction *PI = Worklist.pop_back_val(); @@ -3283,10 +3285,15 @@ static bool isAllocSiteRemovable(Instruction *AI, case Intrinsic::memcpy: case Intrinsic::memset: { MemIntrinsic *MI = cast(II); -if (MI->isVolatile() || MI->getRawDest() != PI) +if (MI->isVolatile()) return false; +// Note: this could also be ModRef, but we can still interpret that as just Mod in that case. +ModRefInfo NewAccess = MI->getRawDest() == PI ? ModRefInfo::Mod : ModRefInfo::Ref; +if ((Access & ~NewAccess) != ModRefInfo::NoModRef) + return false; +Access |= NewAccess; +} [[fallthrough]]; - } case Intrinsic::assume: case Intrinsic::invariant_start: case Intrinsic::invariant_end: @@ -3327,6 +3334,20 @@ static bool isAllocSiteRemovable(Instruction *AI, StoreInst *SI = cast(I); if (SI->isVolatile() || SI->getPointerOperand() != PI) return false; +if (isRefSet(Access)) + return false; +Access |= ModRefInfo::Mod; +Users.emplace_back(I); +continue; + } + + case Instruction::Load: { +LoadInst *LI = cast(I); +if (LI->isVolatile() || LI->getPointerOperand() != LI) + return false; +if (isModSet(Access)) + return false; +Access |= ModRefInfo::Ref; Users.emplace_back(I); continue; } @@ -3334,6 +3355,7 @@ static bool isAllocSiteRemovable(Instruction *AI, llvm_unreachable("missing a return?"); } } while (!Worklist.empty()); + return true; } @@ -3362,10 +3384,23 @@ Instruction *InstCombinerImpl::visitAllocSite(Instruction &MI) { DIB.reset(new DIBuilder(*MI.getModule(), /*AllowUnresolved=*/false)); } - if (isAllocSiteRemovable(&MI, Users, TLI)) { + // Determine what getInitialValueOfAllocation would return without actually allocating the result. + bool KnowInitUndef = isa(MI); + bool KnowInitZero = false; + if (!KnowInitUndef) { +Constant *Init = getInitialValueOfAllocation(&MI, &TLI, Type::getInt8Ty(MI.getContext())); +if (Init) { + if (isa(Init)) +KnowInitUndef = true; + else if (Init->isNullValue()) +KnowInitZero = true; +} + } + + if (isAllocSiteRemovable(&MI, Users, TLI, KnowInitZero | KnowInitUndef)) { for (unsigned i = 0, e = Users.size(); i != e; ++i) { - // Lowering all @llvm.objectsize calls first because they may - // use a bitcast/GEP of the alloca we are removing. + // Lowering all @llvm.objectsize and MTI calls first because they may use + // a bitcast/GEP of the alloca we are removing. if (!Users[i]) continue; @@ -3382,6 +3417,17 @@ Instruction
[clang] [llvm] [InstCombine] remove dead loads, such as memcpy from undef (PR #143958)
@@ -3382,6 +3418,17 @@ Instruction *InstCombinerImpl::visitAllocSite(Instruction &MI) { eraseInstFromFunction(*I); Users[i] = nullptr; // Skip examining in the next loop. } +if (auto *MTI = dyn_cast(I)) { + if (KnowInitZero && getUnderlyingObject(MTI->getRawDest()) != &MI) { +IRBuilderBase::InsertPointGuard Guard(Builder); +Builder.SetInsertPoint(MTI); +auto *M = Builder.CreateMemSet(MTI->getRawDest(), +ConstantInt::get(Type::getInt8Ty(MI.getContext()), 0), +MTI->getLength(), +MTI->getDestAlign()); +M->copyMetadata(*MTI, LLVMContext::MD_DIAssignID); vtjnash wrote: I was wondering about that too. The current MemCpyOptimizer pass usually drops all metadata information (which is where I copied this from to be consistent) but I couldn't really justify that to myself either. https://github.com/llvm/llvm-project/pull/143958 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [InstCombine] remove dead loads, such as memcpy from undef (PR #143958)
https://github.com/vtjnash updated https://github.com/llvm/llvm-project/pull/143958 >From 0d309ccbde665a033976a9eaa9f84c284765825c Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Thu, 12 Jun 2025 18:48:00 + Subject: [PATCH 1/3] [instcombine] remove dead loads, such as memcpy from undef --- .../InstCombine/InstructionCombining.cpp | 66 +-- .../Transforms/InstCombine/malloc-free.ll | 6 +- 2 files changed, 60 insertions(+), 12 deletions(-) diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp index dc2a8cb0115e7..aa7119b005013 100644 --- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp +++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp @@ -3217,10 +3217,12 @@ static bool isRemovableWrite(CallBase &CB, Value *UsedV, static bool isAllocSiteRemovable(Instruction *AI, SmallVectorImpl &Users, - const TargetLibraryInfo &TLI) { + const TargetLibraryInfo &TLI, + bool KnowInit) { SmallVector Worklist; const std::optional Family = getAllocationFamily(AI, &TLI); Worklist.push_back(AI); + ModRefInfo Access = KnowInit ? ModRefInfo::NoModRef : ModRefInfo::Mod; do { Instruction *PI = Worklist.pop_back_val(); @@ -3283,10 +3285,15 @@ static bool isAllocSiteRemovable(Instruction *AI, case Intrinsic::memcpy: case Intrinsic::memset: { MemIntrinsic *MI = cast(II); -if (MI->isVolatile() || MI->getRawDest() != PI) +if (MI->isVolatile()) return false; +// Note: this could also be ModRef, but we can still interpret that as just Mod in that case. +ModRefInfo NewAccess = MI->getRawDest() == PI ? ModRefInfo::Mod : ModRefInfo::Ref; +if ((Access & ~NewAccess) != ModRefInfo::NoModRef) + return false; +Access |= NewAccess; +} [[fallthrough]]; - } case Intrinsic::assume: case Intrinsic::invariant_start: case Intrinsic::invariant_end: @@ -3327,6 +3334,20 @@ static bool isAllocSiteRemovable(Instruction *AI, StoreInst *SI = cast(I); if (SI->isVolatile() || SI->getPointerOperand() != PI) return false; +if (isRefSet(Access)) + return false; +Access |= ModRefInfo::Mod; +Users.emplace_back(I); +continue; + } + + case Instruction::Load: { +LoadInst *LI = cast(I); +if (LI->isVolatile() || LI->getPointerOperand() != LI) + return false; +if (isModSet(Access)) + return false; +Access |= ModRefInfo::Ref; Users.emplace_back(I); continue; } @@ -3334,6 +3355,7 @@ static bool isAllocSiteRemovable(Instruction *AI, llvm_unreachable("missing a return?"); } } while (!Worklist.empty()); + return true; } @@ -3362,10 +3384,23 @@ Instruction *InstCombinerImpl::visitAllocSite(Instruction &MI) { DIB.reset(new DIBuilder(*MI.getModule(), /*AllowUnresolved=*/false)); } - if (isAllocSiteRemovable(&MI, Users, TLI)) { + // Determine what getInitialValueOfAllocation would return without actually allocating the result. + bool KnowInitUndef = isa(MI); + bool KnowInitZero = false; + if (!KnowInitUndef) { +Constant *Init = getInitialValueOfAllocation(&MI, &TLI, Type::getInt8Ty(MI.getContext())); +if (Init) { + if (isa(Init)) +KnowInitUndef = true; + else if (Init->isNullValue()) +KnowInitZero = true; +} + } + + if (isAllocSiteRemovable(&MI, Users, TLI, KnowInitZero | KnowInitUndef)) { for (unsigned i = 0, e = Users.size(); i != e; ++i) { - // Lowering all @llvm.objectsize calls first because they may - // use a bitcast/GEP of the alloca we are removing. + // Lowering all @llvm.objectsize and MTI calls first because they may use + // a bitcast/GEP of the alloca we are removing. if (!Users[i]) continue; @@ -3382,6 +3417,17 @@ Instruction *InstCombinerImpl::visitAllocSite(Instruction &MI) { eraseInstFromFunction(*I); Users[i] = nullptr; // Skip examining in the next loop. } +if (auto *MTI = dyn_cast(I)) { + if (KnowInitZero && getUnderlyingObject(MTI->getRawDest()) != &MI) { +IRBuilderBase::InsertPointGuard Guard(Builder); +Builder.SetInsertPoint(MTI); +auto *M = Builder.CreateMemSet(MTI->getRawDest(), +ConstantInt::get(Type::getInt8Ty(MI.getContext()), 0), +MTI->getLength(), +MTI->getDestAlign()); +M->copyMetadata(*MTI, LLVMContext::MD_DIAssignID); + } +} } } for (unsigned i = 0, e = Users.size(); i != e; ++i) { @@ -3404,7 +3450,13 @@
[clang] [llvm] [InstCombine] remove dead loads, such as memcpy from undef (PR #143958)
@@ -3362,10 +3385,23 @@ Instruction *InstCombinerImpl::visitAllocSite(Instruction &MI) { DIB.reset(new DIBuilder(*MI.getModule(), /*AllowUnresolved=*/false)); } - if (isAllocSiteRemovable(&MI, Users, TLI)) { + // Determine what getInitialValueOfAllocation would return without actually allocating the result. + bool KnowInitUndef = isa(MI); vtjnash wrote: Seems like a good idea, but it doesn't look like it: https://llvm.org/doxygen/namespacellvm.html#a108b6e33f153eda5019d322f0ac909b0 https://github.com/llvm/llvm-project/pull/143958 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [InstCombine] remove dead loads, such as memcpy from undef (PR #143958)
@@ -3283,10 +3285,15 @@ static bool isAllocSiteRemovable(Instruction *AI, case Intrinsic::memcpy: case Intrinsic::memset: { MemIntrinsic *MI = cast(II); -if (MI->isVolatile() || MI->getRawDest() != PI) +if (MI->isVolatile()) return false; +// Note: this could also be ModRef, but we can still interpret that as just Mod in that case. +ModRefInfo NewAccess = MI->getRawDest() == PI ? ModRefInfo::Mod : ModRefInfo::Ref; +if ((Access & ~NewAccess) != ModRefInfo::NoModRef) + return false; vtjnash wrote: I originally was going to have it use AA to compute ModRef at the top, but that seemed potentially slower (since we already know PI aliases). I wasn't sure how else to centralize the test and also preserve exit early unless I just duplicate it at the top and after the loop (`if (Access == ModRefInfo::ModRef) return false`)? https://github.com/llvm/llvm-project/pull/143958 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [InstCombine] remove dead loads, such as memcpy from undef (PR #143958)
https://github.com/vtjnash updated https://github.com/llvm/llvm-project/pull/143958 >From 0d309ccbde665a033976a9eaa9f84c284765825c Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Thu, 12 Jun 2025 18:48:00 + Subject: [PATCH 1/4] [instcombine] remove dead loads, such as memcpy from undef --- .../InstCombine/InstructionCombining.cpp | 66 +-- .../Transforms/InstCombine/malloc-free.ll | 6 +- 2 files changed, 60 insertions(+), 12 deletions(-) diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp index dc2a8cb0115e7..aa7119b005013 100644 --- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp +++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp @@ -3217,10 +3217,12 @@ static bool isRemovableWrite(CallBase &CB, Value *UsedV, static bool isAllocSiteRemovable(Instruction *AI, SmallVectorImpl &Users, - const TargetLibraryInfo &TLI) { + const TargetLibraryInfo &TLI, + bool KnowInit) { SmallVector Worklist; const std::optional Family = getAllocationFamily(AI, &TLI); Worklist.push_back(AI); + ModRefInfo Access = KnowInit ? ModRefInfo::NoModRef : ModRefInfo::Mod; do { Instruction *PI = Worklist.pop_back_val(); @@ -3283,10 +3285,15 @@ static bool isAllocSiteRemovable(Instruction *AI, case Intrinsic::memcpy: case Intrinsic::memset: { MemIntrinsic *MI = cast(II); -if (MI->isVolatile() || MI->getRawDest() != PI) +if (MI->isVolatile()) return false; +// Note: this could also be ModRef, but we can still interpret that as just Mod in that case. +ModRefInfo NewAccess = MI->getRawDest() == PI ? ModRefInfo::Mod : ModRefInfo::Ref; +if ((Access & ~NewAccess) != ModRefInfo::NoModRef) + return false; +Access |= NewAccess; +} [[fallthrough]]; - } case Intrinsic::assume: case Intrinsic::invariant_start: case Intrinsic::invariant_end: @@ -3327,6 +3334,20 @@ static bool isAllocSiteRemovable(Instruction *AI, StoreInst *SI = cast(I); if (SI->isVolatile() || SI->getPointerOperand() != PI) return false; +if (isRefSet(Access)) + return false; +Access |= ModRefInfo::Mod; +Users.emplace_back(I); +continue; + } + + case Instruction::Load: { +LoadInst *LI = cast(I); +if (LI->isVolatile() || LI->getPointerOperand() != LI) + return false; +if (isModSet(Access)) + return false; +Access |= ModRefInfo::Ref; Users.emplace_back(I); continue; } @@ -3334,6 +3355,7 @@ static bool isAllocSiteRemovable(Instruction *AI, llvm_unreachable("missing a return?"); } } while (!Worklist.empty()); + return true; } @@ -3362,10 +3384,23 @@ Instruction *InstCombinerImpl::visitAllocSite(Instruction &MI) { DIB.reset(new DIBuilder(*MI.getModule(), /*AllowUnresolved=*/false)); } - if (isAllocSiteRemovable(&MI, Users, TLI)) { + // Determine what getInitialValueOfAllocation would return without actually allocating the result. + bool KnowInitUndef = isa(MI); + bool KnowInitZero = false; + if (!KnowInitUndef) { +Constant *Init = getInitialValueOfAllocation(&MI, &TLI, Type::getInt8Ty(MI.getContext())); +if (Init) { + if (isa(Init)) +KnowInitUndef = true; + else if (Init->isNullValue()) +KnowInitZero = true; +} + } + + if (isAllocSiteRemovable(&MI, Users, TLI, KnowInitZero | KnowInitUndef)) { for (unsigned i = 0, e = Users.size(); i != e; ++i) { - // Lowering all @llvm.objectsize calls first because they may - // use a bitcast/GEP of the alloca we are removing. + // Lowering all @llvm.objectsize and MTI calls first because they may use + // a bitcast/GEP of the alloca we are removing. if (!Users[i]) continue; @@ -3382,6 +3417,17 @@ Instruction *InstCombinerImpl::visitAllocSite(Instruction &MI) { eraseInstFromFunction(*I); Users[i] = nullptr; // Skip examining in the next loop. } +if (auto *MTI = dyn_cast(I)) { + if (KnowInitZero && getUnderlyingObject(MTI->getRawDest()) != &MI) { +IRBuilderBase::InsertPointGuard Guard(Builder); +Builder.SetInsertPoint(MTI); +auto *M = Builder.CreateMemSet(MTI->getRawDest(), +ConstantInt::get(Type::getInt8Ty(MI.getContext()), 0), +MTI->getLength(), +MTI->getDestAlign()); +M->copyMetadata(*MTI, LLVMContext::MD_DIAssignID); + } +} } } for (unsigned i = 0, e = Users.size(); i != e; ++i) { @@ -3404,7 +3450,13 @@
[clang] [llvm] [InstCombine] remove dead loads, such as memcpy from undef (PR #143958)
https://github.com/vtjnash updated https://github.com/llvm/llvm-project/pull/143958 >From 0d309ccbde665a033976a9eaa9f84c284765825c Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Thu, 12 Jun 2025 18:48:00 + Subject: [PATCH 1/5] [instcombine] remove dead loads, such as memcpy from undef --- .../InstCombine/InstructionCombining.cpp | 66 +-- .../Transforms/InstCombine/malloc-free.ll | 6 +- 2 files changed, 60 insertions(+), 12 deletions(-) diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp index dc2a8cb0115e7..aa7119b005013 100644 --- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp +++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp @@ -3217,10 +3217,12 @@ static bool isRemovableWrite(CallBase &CB, Value *UsedV, static bool isAllocSiteRemovable(Instruction *AI, SmallVectorImpl &Users, - const TargetLibraryInfo &TLI) { + const TargetLibraryInfo &TLI, + bool KnowInit) { SmallVector Worklist; const std::optional Family = getAllocationFamily(AI, &TLI); Worklist.push_back(AI); + ModRefInfo Access = KnowInit ? ModRefInfo::NoModRef : ModRefInfo::Mod; do { Instruction *PI = Worklist.pop_back_val(); @@ -3283,10 +3285,15 @@ static bool isAllocSiteRemovable(Instruction *AI, case Intrinsic::memcpy: case Intrinsic::memset: { MemIntrinsic *MI = cast(II); -if (MI->isVolatile() || MI->getRawDest() != PI) +if (MI->isVolatile()) return false; +// Note: this could also be ModRef, but we can still interpret that as just Mod in that case. +ModRefInfo NewAccess = MI->getRawDest() == PI ? ModRefInfo::Mod : ModRefInfo::Ref; +if ((Access & ~NewAccess) != ModRefInfo::NoModRef) + return false; +Access |= NewAccess; +} [[fallthrough]]; - } case Intrinsic::assume: case Intrinsic::invariant_start: case Intrinsic::invariant_end: @@ -3327,6 +3334,20 @@ static bool isAllocSiteRemovable(Instruction *AI, StoreInst *SI = cast(I); if (SI->isVolatile() || SI->getPointerOperand() != PI) return false; +if (isRefSet(Access)) + return false; +Access |= ModRefInfo::Mod; +Users.emplace_back(I); +continue; + } + + case Instruction::Load: { +LoadInst *LI = cast(I); +if (LI->isVolatile() || LI->getPointerOperand() != LI) + return false; +if (isModSet(Access)) + return false; +Access |= ModRefInfo::Ref; Users.emplace_back(I); continue; } @@ -3334,6 +3355,7 @@ static bool isAllocSiteRemovable(Instruction *AI, llvm_unreachable("missing a return?"); } } while (!Worklist.empty()); + return true; } @@ -3362,10 +3384,23 @@ Instruction *InstCombinerImpl::visitAllocSite(Instruction &MI) { DIB.reset(new DIBuilder(*MI.getModule(), /*AllowUnresolved=*/false)); } - if (isAllocSiteRemovable(&MI, Users, TLI)) { + // Determine what getInitialValueOfAllocation would return without actually allocating the result. + bool KnowInitUndef = isa(MI); + bool KnowInitZero = false; + if (!KnowInitUndef) { +Constant *Init = getInitialValueOfAllocation(&MI, &TLI, Type::getInt8Ty(MI.getContext())); +if (Init) { + if (isa(Init)) +KnowInitUndef = true; + else if (Init->isNullValue()) +KnowInitZero = true; +} + } + + if (isAllocSiteRemovable(&MI, Users, TLI, KnowInitZero | KnowInitUndef)) { for (unsigned i = 0, e = Users.size(); i != e; ++i) { - // Lowering all @llvm.objectsize calls first because they may - // use a bitcast/GEP of the alloca we are removing. + // Lowering all @llvm.objectsize and MTI calls first because they may use + // a bitcast/GEP of the alloca we are removing. if (!Users[i]) continue; @@ -3382,6 +3417,17 @@ Instruction *InstCombinerImpl::visitAllocSite(Instruction &MI) { eraseInstFromFunction(*I); Users[i] = nullptr; // Skip examining in the next loop. } +if (auto *MTI = dyn_cast(I)) { + if (KnowInitZero && getUnderlyingObject(MTI->getRawDest()) != &MI) { +IRBuilderBase::InsertPointGuard Guard(Builder); +Builder.SetInsertPoint(MTI); +auto *M = Builder.CreateMemSet(MTI->getRawDest(), +ConstantInt::get(Type::getInt8Ty(MI.getContext()), 0), +MTI->getLength(), +MTI->getDestAlign()); +M->copyMetadata(*MTI, LLVMContext::MD_DIAssignID); + } +} } } for (unsigned i = 0, e = Users.size(); i != e; ++i) { @@ -3404,7 +3450,13 @@
[clang] [llvm] [InstCombine] remove dead loads, such as memcpy from undef (PR #143958)
vtjnash wrote: I've tried to adjust most of those to avoid alloca, though it is triggering some sort of bug with `simplify_before_foldAndOfICmps` which I'll have to investigate later https://github.com/llvm/llvm-project/pull/143958 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [InstCombine] remove dead loads, such as memcpy from undef (PR #143958)
https://github.com/vtjnash updated https://github.com/llvm/llvm-project/pull/143958 >From 0d309ccbde665a033976a9eaa9f84c284765825c Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Thu, 12 Jun 2025 18:48:00 + Subject: [PATCH 1/7] [instcombine] remove dead loads, such as memcpy from undef --- .../InstCombine/InstructionCombining.cpp | 66 +-- .../Transforms/InstCombine/malloc-free.ll | 6 +- 2 files changed, 60 insertions(+), 12 deletions(-) diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp index dc2a8cb0115e7..aa7119b005013 100644 --- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp +++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp @@ -3217,10 +3217,12 @@ static bool isRemovableWrite(CallBase &CB, Value *UsedV, static bool isAllocSiteRemovable(Instruction *AI, SmallVectorImpl &Users, - const TargetLibraryInfo &TLI) { + const TargetLibraryInfo &TLI, + bool KnowInit) { SmallVector Worklist; const std::optional Family = getAllocationFamily(AI, &TLI); Worklist.push_back(AI); + ModRefInfo Access = KnowInit ? ModRefInfo::NoModRef : ModRefInfo::Mod; do { Instruction *PI = Worklist.pop_back_val(); @@ -3283,10 +3285,15 @@ static bool isAllocSiteRemovable(Instruction *AI, case Intrinsic::memcpy: case Intrinsic::memset: { MemIntrinsic *MI = cast(II); -if (MI->isVolatile() || MI->getRawDest() != PI) +if (MI->isVolatile()) return false; +// Note: this could also be ModRef, but we can still interpret that as just Mod in that case. +ModRefInfo NewAccess = MI->getRawDest() == PI ? ModRefInfo::Mod : ModRefInfo::Ref; +if ((Access & ~NewAccess) != ModRefInfo::NoModRef) + return false; +Access |= NewAccess; +} [[fallthrough]]; - } case Intrinsic::assume: case Intrinsic::invariant_start: case Intrinsic::invariant_end: @@ -3327,6 +3334,20 @@ static bool isAllocSiteRemovable(Instruction *AI, StoreInst *SI = cast(I); if (SI->isVolatile() || SI->getPointerOperand() != PI) return false; +if (isRefSet(Access)) + return false; +Access |= ModRefInfo::Mod; +Users.emplace_back(I); +continue; + } + + case Instruction::Load: { +LoadInst *LI = cast(I); +if (LI->isVolatile() || LI->getPointerOperand() != LI) + return false; +if (isModSet(Access)) + return false; +Access |= ModRefInfo::Ref; Users.emplace_back(I); continue; } @@ -3334,6 +3355,7 @@ static bool isAllocSiteRemovable(Instruction *AI, llvm_unreachable("missing a return?"); } } while (!Worklist.empty()); + return true; } @@ -3362,10 +3384,23 @@ Instruction *InstCombinerImpl::visitAllocSite(Instruction &MI) { DIB.reset(new DIBuilder(*MI.getModule(), /*AllowUnresolved=*/false)); } - if (isAllocSiteRemovable(&MI, Users, TLI)) { + // Determine what getInitialValueOfAllocation would return without actually allocating the result. + bool KnowInitUndef = isa(MI); + bool KnowInitZero = false; + if (!KnowInitUndef) { +Constant *Init = getInitialValueOfAllocation(&MI, &TLI, Type::getInt8Ty(MI.getContext())); +if (Init) { + if (isa(Init)) +KnowInitUndef = true; + else if (Init->isNullValue()) +KnowInitZero = true; +} + } + + if (isAllocSiteRemovable(&MI, Users, TLI, KnowInitZero | KnowInitUndef)) { for (unsigned i = 0, e = Users.size(); i != e; ++i) { - // Lowering all @llvm.objectsize calls first because they may - // use a bitcast/GEP of the alloca we are removing. + // Lowering all @llvm.objectsize and MTI calls first because they may use + // a bitcast/GEP of the alloca we are removing. if (!Users[i]) continue; @@ -3382,6 +3417,17 @@ Instruction *InstCombinerImpl::visitAllocSite(Instruction &MI) { eraseInstFromFunction(*I); Users[i] = nullptr; // Skip examining in the next loop. } +if (auto *MTI = dyn_cast(I)) { + if (KnowInitZero && getUnderlyingObject(MTI->getRawDest()) != &MI) { +IRBuilderBase::InsertPointGuard Guard(Builder); +Builder.SetInsertPoint(MTI); +auto *M = Builder.CreateMemSet(MTI->getRawDest(), +ConstantInt::get(Type::getInt8Ty(MI.getContext()), 0), +MTI->getLength(), +MTI->getDestAlign()); +M->copyMetadata(*MTI, LLVMContext::MD_DIAssignID); + } +} } } for (unsigned i = 0, e = Users.size(); i != e; ++i) { @@ -3404,7 +3450,13 @@
[clang] [llvm] [InstCombine] remove dead loads, such as memcpy from undef (PR #143958)
vtjnash wrote: Sorry for the continued stream of comments, but it turns out this test is supposed to crash, per 41895843b5915bb78e9d02aa711fa10f7174db43, so I've removed the XFAIL and fixed the test invocation to avoid the crash https://github.com/llvm/llvm-project/pull/143958 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [InstCombine] remove dead loads, such as memcpy from undef (PR #143958)
https://github.com/vtjnash updated https://github.com/llvm/llvm-project/pull/143958 >From 0d309ccbde665a033976a9eaa9f84c284765825c Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Thu, 12 Jun 2025 18:48:00 + Subject: [PATCH 1/9] [instcombine] remove dead loads, such as memcpy from undef --- .../InstCombine/InstructionCombining.cpp | 66 +-- .../Transforms/InstCombine/malloc-free.ll | 6 +- 2 files changed, 60 insertions(+), 12 deletions(-) diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp index dc2a8cb0115e7..aa7119b005013 100644 --- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp +++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp @@ -3217,10 +3217,12 @@ static bool isRemovableWrite(CallBase &CB, Value *UsedV, static bool isAllocSiteRemovable(Instruction *AI, SmallVectorImpl &Users, - const TargetLibraryInfo &TLI) { + const TargetLibraryInfo &TLI, + bool KnowInit) { SmallVector Worklist; const std::optional Family = getAllocationFamily(AI, &TLI); Worklist.push_back(AI); + ModRefInfo Access = KnowInit ? ModRefInfo::NoModRef : ModRefInfo::Mod; do { Instruction *PI = Worklist.pop_back_val(); @@ -3283,10 +3285,15 @@ static bool isAllocSiteRemovable(Instruction *AI, case Intrinsic::memcpy: case Intrinsic::memset: { MemIntrinsic *MI = cast(II); -if (MI->isVolatile() || MI->getRawDest() != PI) +if (MI->isVolatile()) return false; +// Note: this could also be ModRef, but we can still interpret that as just Mod in that case. +ModRefInfo NewAccess = MI->getRawDest() == PI ? ModRefInfo::Mod : ModRefInfo::Ref; +if ((Access & ~NewAccess) != ModRefInfo::NoModRef) + return false; +Access |= NewAccess; +} [[fallthrough]]; - } case Intrinsic::assume: case Intrinsic::invariant_start: case Intrinsic::invariant_end: @@ -3327,6 +3334,20 @@ static bool isAllocSiteRemovable(Instruction *AI, StoreInst *SI = cast(I); if (SI->isVolatile() || SI->getPointerOperand() != PI) return false; +if (isRefSet(Access)) + return false; +Access |= ModRefInfo::Mod; +Users.emplace_back(I); +continue; + } + + case Instruction::Load: { +LoadInst *LI = cast(I); +if (LI->isVolatile() || LI->getPointerOperand() != LI) + return false; +if (isModSet(Access)) + return false; +Access |= ModRefInfo::Ref; Users.emplace_back(I); continue; } @@ -3334,6 +3355,7 @@ static bool isAllocSiteRemovable(Instruction *AI, llvm_unreachable("missing a return?"); } } while (!Worklist.empty()); + return true; } @@ -3362,10 +3384,23 @@ Instruction *InstCombinerImpl::visitAllocSite(Instruction &MI) { DIB.reset(new DIBuilder(*MI.getModule(), /*AllowUnresolved=*/false)); } - if (isAllocSiteRemovable(&MI, Users, TLI)) { + // Determine what getInitialValueOfAllocation would return without actually allocating the result. + bool KnowInitUndef = isa(MI); + bool KnowInitZero = false; + if (!KnowInitUndef) { +Constant *Init = getInitialValueOfAllocation(&MI, &TLI, Type::getInt8Ty(MI.getContext())); +if (Init) { + if (isa(Init)) +KnowInitUndef = true; + else if (Init->isNullValue()) +KnowInitZero = true; +} + } + + if (isAllocSiteRemovable(&MI, Users, TLI, KnowInitZero | KnowInitUndef)) { for (unsigned i = 0, e = Users.size(); i != e; ++i) { - // Lowering all @llvm.objectsize calls first because they may - // use a bitcast/GEP of the alloca we are removing. + // Lowering all @llvm.objectsize and MTI calls first because they may use + // a bitcast/GEP of the alloca we are removing. if (!Users[i]) continue; @@ -3382,6 +3417,17 @@ Instruction *InstCombinerImpl::visitAllocSite(Instruction &MI) { eraseInstFromFunction(*I); Users[i] = nullptr; // Skip examining in the next loop. } +if (auto *MTI = dyn_cast(I)) { + if (KnowInitZero && getUnderlyingObject(MTI->getRawDest()) != &MI) { +IRBuilderBase::InsertPointGuard Guard(Builder); +Builder.SetInsertPoint(MTI); +auto *M = Builder.CreateMemSet(MTI->getRawDest(), +ConstantInt::get(Type::getInt8Ty(MI.getContext()), 0), +MTI->getLength(), +MTI->getDestAlign()); +M->copyMetadata(*MTI, LLVMContext::MD_DIAssignID); + } +} } } for (unsigned i = 0, e = Users.size(); i != e; ++i) { @@ -3404,7 +3450,13 @@
[clang] [llvm] [InstCombine] remove dead loads, such as memcpy from undef (PR #143958)
https://github.com/vtjnash updated https://github.com/llvm/llvm-project/pull/143958 >From 450eb079e97f1a20ab2547567d81c176fec06cfb Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Thu, 12 Jun 2025 18:48:00 + Subject: [PATCH 01/12] [instcombine] remove dead loads, such as memcpy from undef --- .../InstCombine/InstructionCombining.cpp | 66 +-- .../Transforms/InstCombine/malloc-free.ll | 6 +- 2 files changed, 60 insertions(+), 12 deletions(-) diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp index afd3359e22ff3..41c8128828281 100644 --- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp +++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp @@ -3279,10 +3279,12 @@ static bool isRemovableWrite(CallBase &CB, Value *UsedV, static bool isAllocSiteRemovable(Instruction *AI, SmallVectorImpl &Users, - const TargetLibraryInfo &TLI) { + const TargetLibraryInfo &TLI, + bool KnowInit) { SmallVector Worklist; const std::optional Family = getAllocationFamily(AI, &TLI); Worklist.push_back(AI); + ModRefInfo Access = KnowInit ? ModRefInfo::NoModRef : ModRefInfo::Mod; do { Instruction *PI = Worklist.pop_back_val(); @@ -3345,10 +3347,15 @@ static bool isAllocSiteRemovable(Instruction *AI, case Intrinsic::memcpy: case Intrinsic::memset: { MemIntrinsic *MI = cast(II); -if (MI->isVolatile() || MI->getRawDest() != PI) +if (MI->isVolatile()) return false; +// Note: this could also be ModRef, but we can still interpret that as just Mod in that case. +ModRefInfo NewAccess = MI->getRawDest() == PI ? ModRefInfo::Mod : ModRefInfo::Ref; +if ((Access & ~NewAccess) != ModRefInfo::NoModRef) + return false; +Access |= NewAccess; +} [[fallthrough]]; - } case Intrinsic::assume: case Intrinsic::invariant_start: case Intrinsic::invariant_end: @@ -3389,6 +3396,20 @@ static bool isAllocSiteRemovable(Instruction *AI, StoreInst *SI = cast(I); if (SI->isVolatile() || SI->getPointerOperand() != PI) return false; +if (isRefSet(Access)) + return false; +Access |= ModRefInfo::Mod; +Users.emplace_back(I); +continue; + } + + case Instruction::Load: { +LoadInst *LI = cast(I); +if (LI->isVolatile() || LI->getPointerOperand() != LI) + return false; +if (isModSet(Access)) + return false; +Access |= ModRefInfo::Ref; Users.emplace_back(I); continue; } @@ -3396,6 +3417,7 @@ static bool isAllocSiteRemovable(Instruction *AI, llvm_unreachable("missing a return?"); } } while (!Worklist.empty()); + return true; } @@ -3424,10 +3446,23 @@ Instruction *InstCombinerImpl::visitAllocSite(Instruction &MI) { DIB.reset(new DIBuilder(*MI.getModule(), /*AllowUnresolved=*/false)); } - if (isAllocSiteRemovable(&MI, Users, TLI)) { + // Determine what getInitialValueOfAllocation would return without actually allocating the result. + bool KnowInitUndef = isa(MI); + bool KnowInitZero = false; + if (!KnowInitUndef) { +Constant *Init = getInitialValueOfAllocation(&MI, &TLI, Type::getInt8Ty(MI.getContext())); +if (Init) { + if (isa(Init)) +KnowInitUndef = true; + else if (Init->isNullValue()) +KnowInitZero = true; +} + } + + if (isAllocSiteRemovable(&MI, Users, TLI, KnowInitZero | KnowInitUndef)) { for (unsigned i = 0, e = Users.size(); i != e; ++i) { - // Lowering all @llvm.objectsize calls first because they may - // use a bitcast/GEP of the alloca we are removing. + // Lowering all @llvm.objectsize and MTI calls first because they may use + // a bitcast/GEP of the alloca we are removing. if (!Users[i]) continue; @@ -3444,6 +3479,17 @@ Instruction *InstCombinerImpl::visitAllocSite(Instruction &MI) { eraseInstFromFunction(*I); Users[i] = nullptr; // Skip examining in the next loop. } +if (auto *MTI = dyn_cast(I)) { + if (KnowInitZero && getUnderlyingObject(MTI->getRawDest()) != &MI) { +IRBuilderBase::InsertPointGuard Guard(Builder); +Builder.SetInsertPoint(MTI); +auto *M = Builder.CreateMemSet(MTI->getRawDest(), +ConstantInt::get(Type::getInt8Ty(MI.getContext()), 0), +MTI->getLength(), +MTI->getDestAlign()); +M->copyMetadata(*MTI, LLVMContext::MD_DIAssignID); + } +} } } for (unsigned i = 0, e = Users.size(); i != e; ++i) { @@ -3466,7 +3512,13 @
[clang] [llvm] [InstCombine] remove dead loads, such as memcpy from undef (PR #143958)
https://github.com/vtjnash updated https://github.com/llvm/llvm-project/pull/143958 >From 450eb079e97f1a20ab2547567d81c176fec06cfb Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Thu, 12 Jun 2025 18:48:00 + Subject: [PATCH 01/12] [instcombine] remove dead loads, such as memcpy from undef --- .../InstCombine/InstructionCombining.cpp | 66 +-- .../Transforms/InstCombine/malloc-free.ll | 6 +- 2 files changed, 60 insertions(+), 12 deletions(-) diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp index afd3359e22ff3..41c8128828281 100644 --- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp +++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp @@ -3279,10 +3279,12 @@ static bool isRemovableWrite(CallBase &CB, Value *UsedV, static bool isAllocSiteRemovable(Instruction *AI, SmallVectorImpl &Users, - const TargetLibraryInfo &TLI) { + const TargetLibraryInfo &TLI, + bool KnowInit) { SmallVector Worklist; const std::optional Family = getAllocationFamily(AI, &TLI); Worklist.push_back(AI); + ModRefInfo Access = KnowInit ? ModRefInfo::NoModRef : ModRefInfo::Mod; do { Instruction *PI = Worklist.pop_back_val(); @@ -3345,10 +3347,15 @@ static bool isAllocSiteRemovable(Instruction *AI, case Intrinsic::memcpy: case Intrinsic::memset: { MemIntrinsic *MI = cast(II); -if (MI->isVolatile() || MI->getRawDest() != PI) +if (MI->isVolatile()) return false; +// Note: this could also be ModRef, but we can still interpret that as just Mod in that case. +ModRefInfo NewAccess = MI->getRawDest() == PI ? ModRefInfo::Mod : ModRefInfo::Ref; +if ((Access & ~NewAccess) != ModRefInfo::NoModRef) + return false; +Access |= NewAccess; +} [[fallthrough]]; - } case Intrinsic::assume: case Intrinsic::invariant_start: case Intrinsic::invariant_end: @@ -3389,6 +3396,20 @@ static bool isAllocSiteRemovable(Instruction *AI, StoreInst *SI = cast(I); if (SI->isVolatile() || SI->getPointerOperand() != PI) return false; +if (isRefSet(Access)) + return false; +Access |= ModRefInfo::Mod; +Users.emplace_back(I); +continue; + } + + case Instruction::Load: { +LoadInst *LI = cast(I); +if (LI->isVolatile() || LI->getPointerOperand() != LI) + return false; +if (isModSet(Access)) + return false; +Access |= ModRefInfo::Ref; Users.emplace_back(I); continue; } @@ -3396,6 +3417,7 @@ static bool isAllocSiteRemovable(Instruction *AI, llvm_unreachable("missing a return?"); } } while (!Worklist.empty()); + return true; } @@ -3424,10 +3446,23 @@ Instruction *InstCombinerImpl::visitAllocSite(Instruction &MI) { DIB.reset(new DIBuilder(*MI.getModule(), /*AllowUnresolved=*/false)); } - if (isAllocSiteRemovable(&MI, Users, TLI)) { + // Determine what getInitialValueOfAllocation would return without actually allocating the result. + bool KnowInitUndef = isa(MI); + bool KnowInitZero = false; + if (!KnowInitUndef) { +Constant *Init = getInitialValueOfAllocation(&MI, &TLI, Type::getInt8Ty(MI.getContext())); +if (Init) { + if (isa(Init)) +KnowInitUndef = true; + else if (Init->isNullValue()) +KnowInitZero = true; +} + } + + if (isAllocSiteRemovable(&MI, Users, TLI, KnowInitZero | KnowInitUndef)) { for (unsigned i = 0, e = Users.size(); i != e; ++i) { - // Lowering all @llvm.objectsize calls first because they may - // use a bitcast/GEP of the alloca we are removing. + // Lowering all @llvm.objectsize and MTI calls first because they may use + // a bitcast/GEP of the alloca we are removing. if (!Users[i]) continue; @@ -3444,6 +3479,17 @@ Instruction *InstCombinerImpl::visitAllocSite(Instruction &MI) { eraseInstFromFunction(*I); Users[i] = nullptr; // Skip examining in the next loop. } +if (auto *MTI = dyn_cast(I)) { + if (KnowInitZero && getUnderlyingObject(MTI->getRawDest()) != &MI) { +IRBuilderBase::InsertPointGuard Guard(Builder); +Builder.SetInsertPoint(MTI); +auto *M = Builder.CreateMemSet(MTI->getRawDest(), +ConstantInt::get(Type::getInt8Ty(MI.getContext()), 0), +MTI->getLength(), +MTI->getDestAlign()); +M->copyMetadata(*MTI, LLVMContext::MD_DIAssignID); + } +} } } for (unsigned i = 0, e = Users.size(); i != e; ++i) { @@ -3466,7 +3512,13 @
[clang] [llvm] [InstCombine] remove dead loads, such as memcpy from undef (PR #143958)
https://github.com/vtjnash closed https://github.com/llvm/llvm-project/pull/143958 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits