`clamr_cpuonly.cpp` 53% compile time slowdown under -O0

[bcardosolopes]

As presented in AsiaLLVM, one edge case for bad compile performance is clamr_cpuonly.cpp (53% slowdown under -O0) from Multisource benchmark.

The talk describes the bottleneck in CIRGenModule::applyReplacements(), divided in replacePointerTypeArgs (50%) and replaceAllSymbolUses (50%), while dealing dtor/ctor completion and replacement by aliasee’s.

[rafasumi]

I'd like to work on this!

One question. Can different calls to the same function have arguments of different types? If not, this nested for loop can probably be hoisted and be applied just once per function:

clangir/clang/lib/CIR/CodeGen/CIRGenModule.cpp

Lines 3941 to 3946 in 90fc6c4

	for (unsigned i = 0; i < funcArgTypes.size(); i++) {
	if (argOps[i].getType() == funcArgTypes[i])
	continue;
	auto argPointerTy = mlir::dyn_cast<cir::PointerType>(argOps[i].getType());
	auto funcArgPointerTy = mlir::dyn_cast<cir::PointerType>(funcArgTypes[i]);

[bcardosolopes]

Can different calls to the same function have arguments of different types?

Yes. We might need some book keeping or do some of this early on to fix the issue, I haven't looked in depth

[rafasumi]

I'll take a deeper look at this when I when some time. Thanks!

[rafasumi]

Hey @bcardosolopes, sorry for the delay. I'm busy with some other things, but I've been inspecting this issue in my spare time. I haven't come up with anything substantial yet though.

First of all, I think replacePointerTypeArgs seems to be a better candidate for improvements than replaceAllSymbolUses. I've been trying to come up with ideas to make some of this processing early on, but I'm a bit stuck. The only idea that I had was to keep track of all cir::CallOps that have a cir::FuncOp as callee in the cir::FuncOp itself. That way, we can avoid having to traverse the range of symbol uses to find the calls to a function:

clangir/clang/lib/CIR/CodeGen/CIRGenModule.cpp

Lines 3929 to 3937 in 822684e

	auto optionalUseRange = oldF.getSymbolUses(theModule);
	if (!optionalUseRange)
	return;
	for (auto u : *optionalUseRange) {
	// CallTryOp only shows up after FlattenCFG.
	auto call = mlir::dyn_cast<cir::CallOp>(u.getUser());
	if (!call)
	continue;

However, I don't think this would lead to any significant performance improvements, since most uses to the symbol would be calls anyway. Do you have any other suggestions of what might be worth exploring to improve replacePointerTypeArgs?

On a different note: I wanted to reproduce the llvm-test-suite results from the AsiaLLVM talk. Can I reproduce it by just comparing the results for O0 and O2 when running Multisource with -DTEST_SUITE_RUN_BENCHMARKS=OFF? Like this:

cmake \
  -DCMAKE_C_COMPILER=<clangir-dir>/clang \
  CMAKE_C_FLAGS="-fclangir" \
  -DTEST_SUITE_RUN_BENCHMARKS=OFF \
  -DTEST_SUITE_SUBDIRS=MultiSource/  \
  -C../cmake/caches/O2.cmake \
  -GNinja \
  ../

llvm-lit -v -j 1 -o results_O2.json .

[bcardosolopes]

The only idea that I had was to keep track of all cir::CallOps that have a cir::FuncOp as callee in the cir::FuncOp itself

If we need to do bookkeeping, we shouldn't be adding state to any CIR operation, but in CGM instead.

On a different note: I wanted to reproduce the llvm-test-suite results from the AsiaLLVM talk. Can I reproduce it by just comparing the results for O0 and O2 when running Multisource with -DTEST_SUITE_RUN_BENCHMARKS=OFF?

You only need to look at -O0 for this issue, ../cmake/caches/O0.cmake would be more appropriate. You also need to run ninja before running llvm-lit. Once you get ninja working, you can extra the build invocation for clamr_cpuonly.cpp and first measure it yourself to confirm the difference, and then you go from there to experiment.

[rafasumi]

Hey @bcardosolopes. I was able to implement this idea of bookkeeping calls to dtor/ctor FuncOps. However, I wasn't able to build clamr_cpuonly.cpp when running llvm-test-suite with CIR code gen enabled. Even when running from the main branch, it crashes due to a failed assertion:

clang++: /home/rafael/Git/clangir/mlir/lib/IR/Operation.cpp:563: void mlir::Operation::moveBefore(Block *, llvm::iplist<Operation>::iterator): Assertion `getBlock() && "cannot move an operation that isn't contained in a block"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: /home/rafael/Git/clangir/build/bin/clang++ -DNDEBUG -O2 -fclangir -w -Werror=date-time -save-stats=obj -ffp-contract=off -DFMA_DISABLED=1 -save-stats=obj -ffp-contract=off -DFMA_DISABLED=1 -MD -MT MultiSource/Benchmarks/DOE-ProxyApps-C++/CLAMR/CMakeFiles/CLAMR.dir/clamr_cpuonly.cpp.o -MF MultiSource/Benchmarks/DOE-ProxyApps-C++/CLAMR/CMakeFiles/CLAMR.dir/clamr_cpuonly.cpp.o.d -o MultiSource/Benchmarks/DOE-ProxyApps-C++/CLAMR/CMakeFiles/CLAMR.dir/clamr_cpuonly.cpp.o -c /home/rafael/Git/llvm-test-suite/MultiSource/Benchmarks/DOE-ProxyApps-C++/CLAMR/clamr_cpuonly.cpp
1.      <eof> parser at end of file
 #0 0x00005f8033e14758 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/home/rafael/Git/clangir/build/bin/clang+++0x2595758)
 #1 0x00005f8033e11ed5 llvm::sys::RunSignalHandlers() (/home/rafael/Git/clangir/build/bin/clang+++0x2592ed5)
 #2 0x00005f8033d83f46 CrashRecoverySignalHandler(int) CrashRecoveryContext.cpp:0:0
 #3 0x00007b0efa842520 (/lib/x86_64-linux-gnu/libc.so.6+0x42520)
 #4 0x00007b0efa8969fc pthread_kill (/lib/x86_64-linux-gnu/libc.so.6+0x969fc)
 #5 0x00007b0efa842476 gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x42476)
 #6 0x00007b0efa8287f3 abort (/lib/x86_64-linux-gnu/libc.so.6+0x287f3)
 #7 0x00007b0efa82871b (/lib/x86_64-linux-gnu/libc.so.6+0x2871b)
 #8 0x00007b0efa839e96 (/lib/x86_64-linux-gnu/libc.so.6+0x39e96)
 #9 0x00005f8036e23024 (/home/rafael/Git/clangir/build/bin/clang+++0x55a4024)
#10 0x00005f8034d9e412 clang::CIRGen::CIRGenModule::applyReplacements() (/home/rafael/Git/clangir/build/bin/clang+++0x351f412)
#11 0x00005f8034d9d5e3 clang::CIRGen::CIRGenModule::Release() (/home/rafael/Git/clangir/build/bin/clang+++0x351e5e3)
#12 0x00005f8034c7092c cir::CIRGenConsumer::HandleTranslationUnit(clang::ASTContext&) (/home/rafael/Git/clangir/build/bin/clang+++0x33f192c)
#13 0x00005f80372a5ad9 clang::ParseAST(clang::Sema&, bool, bool) (/home/rafael/Git/clangir/build/bin/clang+++0x5a26ad9)
#14 0x00005f803488fc56 clang::FrontendAction::Execute() (/home/rafael/Git/clangir/build/bin/clang+++0x3010c56)
#15 0x00005f80347fd21d clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (/home/rafael/Git/clangir/build/bin/clang+++0x2f7e21d)
#16 0x00005f803496a34f clang::ExecuteCompilerInvocation(clang::CompilerInstance*) (/home/rafael/Git/clangir/build/bin/clang+++0x30eb34f)
#17 0x00005f8032b64c9b cc1_main(llvm::ArrayRef<char const*>, char const*, void*) (/home/rafael/Git/clangir/build/bin/clang+++0x12e5c9b)
#18 0x00005f8032b6111f ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&, llvm::ToolContext const&) driver.cpp:0:0
#19 0x00005f8034659c69 void llvm::function_ref<void ()>::callback_fn<clang::driver::CC1Command::Execute(llvm::ArrayRef<std::optional<llvm::StringRef>>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>*, bool*) const::$_0>(long) Job.cpp:0:0
#20 0x00005f8033d83bee llvm::CrashRecoveryContext::RunSafely(llvm::function_ref<void ()>) (/home/rafael/Git/clangir/build/bin/clang+++0x2504bee)
#21 0x00005f80346595b3 clang::driver::CC1Command::Execute(llvm::ArrayRef<std::optional<llvm::StringRef>>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>*, bool*) const (/home/rafael/Git/clangir/build/bin/clang+++0x2dda5b3)
#22 0x00005f8034615dac clang::driver::Compilation::ExecuteCommand(clang::driver::Command const&, clang::driver::Command const*&, bool) const (/home/rafael/Git/clangir/build/bin/clang+++0x2d96dac)
#23 0x00005f8034615fbe clang::driver::Compilation::ExecuteJobs(clang::driver::JobList const&, llvm::SmallVectorImpl<std::pair<int, clang::driver::Command const*>>&, bool) const (/home/rafael/Git/clangir/build/bin/clang+++0x2d96fbe)
#24 0x00005f80346360dd clang::driver::Driver::ExecuteCompilation(clang::driver::Compilation&, llvm::SmallVectorImpl<std::pair<int, clang::driver::Command const*>>&) (/home/rafael/Git/clangir/build/bin/clang+++0x2db70dd)
#25 0x00005f8032b60a1c clang_main(int, char**, llvm::ToolContext const&) (/home/rafael/Git/clangir/build/bin/clang+++0x12e1a1c)
#26 0x00005f8032b6fc07 main (/home/rafael/Git/clangir/build/bin/clang+++0x12f0c07)
#27 0x00007b0efa829d90 (/lib/x86_64-linux-gnu/libc.so.6+0x29d90)
#28 0x00007b0efa829e40 __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e40)
#29 0x00005f8032b5ec25 _start (/home/rafael/Git/clangir/build/bin/clang+++0x12dfc25)

I'm building it with -O2 -fclangir, after having built everything else with -O0.

Were the tests originally run with assertions disabled? Or is this new?

[bcardosolopes]

I just build it locally and seems like we indeed regressed (-O0 has a different crash than -O2), fixing these crashes are probably a good beginner issue if you are interested.

[rafasumi]

Sounds good to me! What was the crash for -O0? I can create the issues if you prefer.

[bcardosolopes]

Sounds good to me! What was the crash for -O0? I can create the issues if you prefer.

I don't recall anymore (lost the build dir), but should be straightforward to repro if you are interested in following up

[rafasumi]

I've created an issue for the crash on -O2: #1938. I haven't been able to reproduce the failure you mentioned on -O0 though.

[bcardosolopes]

I haven't been able to reproduce the failure you mentioned on -O0 though.

Maybe it's gone already, which means it's possible to investigate the compile time issue again. Note that some crashes are only seen when clang is built with asserts ON, in case you are using release-noasserts

[rafasumi]

Maybe it's gone already, which means it's possible to investigate the compile time issue again. Note that some crashes are only seen when clang is built with asserts ON, in case you are using release-noasserts

Although I was testing with assertions ON, I tested it again with a clean build just to be sure. Again, no crashes on -O0, but -O2 is still failing. I'll address this crash before moving on to the compile time issue.