Page MenuHomePhabricator

[LegacyPM] Rename and deprecate populateModulePassManager
Needs ReviewPublic

Authored by MaskRay on Apr 19 2022, 9:47 PM.

Details

Summary

Using LLVM's legacy PM for optimization pipeline was deprecated in
13.0.0
and some rarely used pieces are being removed.

populateModulePassManager is a main API indicating legacy usage. Rename
and deprecate it to make users aware. Use a function attribute instead
of LLVM_ATTRIBUTE_DEPRECATED to allow warning suppression with #pragma
without worrying about MSVC.

Diff Detail

Unit TestsFailed

TimeTest
60,540 msx64 debian > SanitizerCommon-msan-x86_64-Linux.Linux::soft_rss_limit_mb_test.cpp
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=memory -m64 -funwind-tables -ldl -O2 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/soft_rss_limit_mb_test.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/sanitizer_common/msan-x86_64-Linux/Linux/Output/soft_rss_limit_mb_test.cpp.tmp
60,250 msx64 debian > SanitizerCommon-tsan-x86_64-Linux.Linux::soft_rss_limit_mb_test.cpp
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=thread -m64 -funwind-tables -ldl -O2 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/soft_rss_limit_mb_test.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/sanitizer_common/tsan-x86_64-Linux/Linux/Output/soft_rss_limit_mb_test.cpp.tmp
60,260 msx64 debian > SanitizerCommon-ubsan-x86_64-Linux.Linux::soft_rss_limit_mb_test.cpp
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=undefined -m64 -funwind-tables -ldl -O2 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/soft_rss_limit_mb_test.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/sanitizer_common/ubsan-x86_64-Linux/Linux/Output/soft_rss_limit_mb_test.cpp.tmp
60,180 msx64 debian > Scudo-x86_64.Scudo-x86_64::rss.c
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -m64 -pthread -fPIE -pie -O0 -UNDEBUG -ldl -Wl,--gc-sections -lrt -fsanitize=scudo /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/scudo/rss.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/scudo/X86_64LinuxConfig/Output/rss.c.tmp
60,140 msx64 debian > ScudoStandalone-Unit-GwpAsanTorture._/ScudoUnitTest-x86_64-Test/2::54
Script(shard): -- GTEST_COLOR=no
View Full Test Results (10 Failed)

Event Timeline

MaskRay created this revision.Apr 19 2022, 9:47 PM
Herald added a reviewer: bollu. · View Herald Transcript
Herald added a reviewer: ctetreau. · View Herald Transcript
Herald added a reviewer: ctetreau. · View Herald Transcript
Herald added a reviewer: ctetreau. · View Herald Transcript
Herald added a reviewer: ctetreau. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay requested review of this revision.Apr 19 2022, 9:47 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 19 2022, 9:47 PM

Please do not change names in the FFI API at least. It's okay to drop it entirely, but renaming is unnecessarily hostile for FFI APIs.

clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp
95

This seems like an easy usage to replace?

llvm/include/llvm-c/Transforms/PassManagerBuilder.h
72

Let's not promise any specific timeline. IMHO we should drop this as soon as there are no in-tree users anymore.

llvm/unittests/ExecutionEngine/MCJIT/MCJITCAPITest.cpp
317

This also looks like an easy usage to remove.

MaskRay marked an inline comment as done.Apr 20 2022, 12:36 AM

Please do not change names in the FFI API at least. It's okay to drop it entirely, but renaming is unnecessarily hostile for FFI APIs.

Why?

Please do not change names in the FFI API at least. It's okay to drop it entirely, but renaming is unnecessarily hostile for FFI APIs.

Why?

Users of the FFI interface generally target multiple LLVM versions, so they need to introduce checks to use one or the other API -- if lucky only compile-time checks, if not lucky it requires switching the code to use dlsym. This may be unavoidable if an API is removed, but it's an unnecessary complication for a rename.

MaskRay updated this revision to Diff 424374.Apr 21 2022, 9:35 PM

I still do not see the argument about C API, but I just drop that changes.

I'd first fix up as many in-tree users as nikic pointed out

(I'll work on bugpoint soonish since that requires a bit more work)

llvm/include/llvm-c/Transforms/PassManagerBuilder.h
72

can we use llvm/include/llvm-c/Deprecated.h for the C API here? looks like we only have on in-tree user in llvm/unittests/ExecutionEngine/MCJIT/MCJITCAPITest.cpp that we should be able to clean up

Apologies if this isn't directly related to this patch in particular, but is there a more informative message that could be given than "Migrate to new pass manager"? In particular, for non-C language bindings that currently call LLVMPassManagerBuilderPopulateModulePassManager, any pointers to what functions in the C API should be used instead would be very helpful.

ormris removed a subscriber: ormris.May 16 2022, 11:27 AM