Differential D156016
[Support] Change MapVector's default template parameter to SmallVector<*, 0> Authored by MaskRay on Jul 21 2023, 7:59 PM.
Details
SmallVector<*, 0> is often a better replacement for std::vector : clang size decreases by 0.0226%.
Diff Detail
Event TimelineComment Actions Makes sense: LGTM. Slightly tangential: If we want to encourage more use of SmallVector, would it make sense to make the second template argument default to 0 so that folks can just write SmallVector<T> or do we want to keep it explicit so users are forced to pick a sensible value? Comment Actions Thank you! Yes, I think it would be nice that the default parameter is 0, but a while ago it was changed to mean like 64/sizeof(T)... Comment Actions This patch causes compile error when using GCC 7.5.0. [303/2700] Building CXX object lib/CodeGen/CMakeFiles/LLVMCodeGen.dir/FaultMaps.cpp.o
FAILED: lib/CodeGen/CMakeFiles/LLVMCodeGen.dir/FaultMaps.cpp.o
ccache /usr/bin/c++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D_LIBCPP_ENABLE_ASSERTIONS -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/gpfs/jlse-fs0/users/ac.shilei.tian/build/llvm/release/lib/CodeGen -I/home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/lib/CodeGen -I/gpfs/jlse-fs0/users/ac.shilei.tian/build/llvm/release/include -I/home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -fno-lifetime-dse -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-nonnull -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wno-comment -Wno-misleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG -fPIC -fno-exceptions -funwind-tables -fno-rtti -UNDEBUG -std=c++1z -MD -MT lib/CodeGen/CMakeFiles/LLVMCodeGen.dir/FaultMaps.cpp.o -MF lib/CodeGen/CMakeFiles/LLVMCodeGen.dir/FaultMaps.cpp.o.d -o lib/CodeGen/CMakeFiles/LLVMCodeGen.dir/FaultMaps.cpp.o -c /home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/lib/CodeGen/FaultMaps.cpp
In file included from /home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/include/llvm/Support/PointerLikeTypeTraits.h:19:0,
from /home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/include/llvm/ADT/PointerIntPair.h:18,
from /home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/include/llvm/MC/MCSymbol.h:16,
from /home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/include/llvm/CodeGen/FaultMaps.h:12,
from /home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/lib/CodeGen/FaultMaps.cpp:9:
/usr/include/c++/7/type_traits: In substitution of ‘template<class _Tp, class> static std::true_type std::__do_is_default_constructible_impl::__test(int) [with _Tp = llvm::StackMaps::FunctionInfo; <template-parameter-1-2> = <missing>]’:
/usr/include/c++/7/type_traits:889:35: required from ‘struct std::__is_default_constructible_impl<llvm::StackMaps::FunctionInfo>’
/usr/include/c++/7/type_traits:143:12: required from ‘struct std::__and_<std::__not_<std::is_void<llvm::StackMaps::FunctionInfo> >, std::__is_default_constructible_impl<llvm::StackMaps::FunctionInfo> >’
/usr/include/c++/7/type_traits:893:12: required from ‘struct std::__is_default_constructible_atom<llvm::StackMaps::FunctionInfo>’
/usr/include/c++/7/type_traits:914:12: required from ‘struct std::__is_default_constructible_safe<llvm::StackMaps::FunctionInfo, false>’
/usr/include/c++/7/type_traits:920:12: required from ‘struct std::is_default_constructible<llvm::StackMaps::FunctionInfo>’
/usr/include/c++/7/type_traits:143:12: [ skipping 16 instantiation contexts, use -ftemplate-backtrace-limit=0 to disable ]
/usr/include/c++/7/type_traits:1147:12: required from ‘struct std::is_copy_constructible<llvm::detail::copy_construction_triviality_helper<std::pair<const llvm::MCSymbol*, llvm::StackMaps::FunctionInfo> > >’
/home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/include/llvm/Support/type_traits.h:97:8: required from ‘struct llvm::is_trivially_copy_constructible<std::pair<const llvm::MCSymbol*, llvm::StackMaps::FunctionInfo> >’
/home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/include/llvm/ADT/SmallVector.h:329:74: required from ‘class llvm::SmallVectorImpl<std::pair<const llvm::MCSymbol*, llvm::StackMaps::FunctionInfo> >’
/home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/include/llvm/ADT/SmallVector.h:1199:22: required from ‘class llvm::SmallVector<std::pair<const llvm::MCSymbol*, llvm::StackMaps::FunctionInfo>, 0>’
/home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/include/llvm/ADT/MapVector.h:38:14: required from ‘class llvm::MapVector<const llvm::MCSymbol*, llvm::StackMaps::FunctionInfo>’
/home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/include/llvm/CodeGen/StackMaps.h:367:13: required from here
/usr/include/c++/7/type_traits:878:48: error: constructor required before non-static data member for ‘llvm::StackMaps::FunctionInfo::StackSize’ has been parsed
template<typename _Tp, typename = decltype(_Tp())>
^~~~~
/usr/include/c++/7/type_traits:878:48: error: constructor required before non-static data member for ‘llvm::StackMaps::FunctionInfo::RecordCount’ has been parsedComment Actions And also under gcc 9.4.0 -- this has broken Halide's integration (which relies on older gcc for some platforms), so if a fix is not imminent, could this be rolled back pending a fix? Comment Actions --- i/llvm/include/llvm/CodeGen/StackMaps.h +++ w/llvm/include/llvm/CodeGen/StackMaps.h @@ -330,7 +330,9 @@ public: LiveOuts(std::move(LiveOuts)) {} }; - using FnInfoMap = MapVector<const MCSymbol *, FunctionInfo>; + using FnInfoMap = MapVector<const MCSymbol *, FunctionInfo, + DenseMap<const MCSymbol *, unsigned>, + std::vector<FunctionInfo>>; using CallsiteInfoList = std::vector<CallsiteInfo>; /// Generate a stackmap record for a stackmap instruction. can work around gcc 7.3.0, but it seems that gcc 7.3.0 has other errors unrelated to MapVector. Comment Actions Are you gonna push the change? Since LLVM supports GCC 7.1.0, the code needs to be compiled with GCC 7.3.0 or 7.5.0. Comment Actions I am actively analyzing this instance. I think we should reland D132311 (Remove llvm::is_trivially_{copy/move}_constructible), which will fix this build error for GCC 7.3.0/7.5.0. However, GCC 7.3.0 has many other build failures related to structured binding. Perhaps we should bump the GCC version requirement to at least 7.5.0. Comment Actions This doesn't seem to be able to work around GCC 7.5.0. /home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/lib/CodeGen/StackMaps.cpp: In member function ‘void llvm::StackMaps::recordStackMapOpers(const llvm::MCSymbol&, const llvm::MachineInstr&, uint64_t, llvm::MachineInstr::const_mop_iterator, llvm::MachineInstr::const_mop_iterator, bool)’:
/home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/lib/CodeGen/StackMaps.cpp:539:16: error: ‘struct llvm::StackMaps::FunctionInfo’ has no member named ‘second’
CurrentIt->second.RecordCount++;
^~~~~~
In file included from /home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/include/llvm/CodeGen/LowLevelType.h:31:0,
from /home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/include/llvm/CodeGen/MachineMemOperand.h:20,
from /home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/include/llvm/CodeGen/MachineInstr.h:24,
from /home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/include/llvm/CodeGen/StackMaps.h:14,
from /home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/lib/CodeGen/StackMaps.cpp:9:
/home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/lib/CodeGen/StackMaps.cpp: In member function ‘void llvm::StackMaps::emitFunctionFrameRecords(llvm::MCStreamer&)’:
/home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/lib/CodeGen/StackMaps.cpp:622:58: error: ‘const struct llvm::StackMaps::FunctionInfo’ has no member named ‘first’
LLVM_DEBUG(dbgs() << WSMP << "function addr: " << FR.first
^
/home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/include/llvm/Support/Debug.h:65:69: note: in definition of macro ‘DEBUG_WITH_TYPE’
do { if (::llvm::DebugFlag && ::llvm::isCurrentDebugType(TYPE)) { X; } \
^
/home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/lib/CodeGen/StackMaps.cpp:622:5: note: in expansion of macro ‘LLVM_DEBUG’
LLVM_DEBUG(dbgs() << WSMP << "function addr: " << FR.first
^~~~~~~~~~
/home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/lib/CodeGen/StackMaps.cpp:623:48: error: ‘const struct llvm::StackMaps::FunctionInfo’ has no member named ‘second’
<< " frame size: " << FR.second.StackSize
^
/home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/include/llvm/Support/Debug.h:65:69: note: in definition of macro ‘DEBUG_WITH_TYPE’
do { if (::llvm::DebugFlag && ::llvm::isCurrentDebugType(TYPE)) { X; } \
^
/home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/lib/CodeGen/StackMaps.cpp:622:5: note: in expansion of macro ‘LLVM_DEBUG’
LLVM_DEBUG(dbgs() << WSMP << "function addr: " << FR.first
^~~~~~~~~~
/home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/lib/CodeGen/StackMaps.cpp:624:52: error: ‘const struct llvm::StackMaps::FunctionInfo’ has no member named ‘second’
<< " callsite count: " << FR.second.RecordCount << '\n');
^
/home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/include/llvm/Support/Debug.h:65:69: note: in definition of macro ‘DEBUG_WITH_TYPE’
do { if (::llvm::DebugFlag && ::llvm::isCurrentDebugType(TYPE)) { X; } \
^
/home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/lib/CodeGen/StackMaps.cpp:622:5: note: in expansion of macro ‘LLVM_DEBUG’
LLVM_DEBUG(dbgs() << WSMP << "function addr: " << FR.first
^~~~~~~~~~
/home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/lib/CodeGen/StackMaps.cpp:625:27: error: ‘const struct llvm::StackMaps::FunctionInfo’ has no member named ‘first’
OS.emitSymbolValue(FR.first, 8);
^~~~~
/home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/lib/CodeGen/StackMaps.cpp:626:24: error: ‘const struct llvm::StackMaps::FunctionInfo’ has no member named ‘second’
OS.emitIntValue(FR.second.StackSize, 8);
^~~~~~
/home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/lib/CodeGen/StackMaps.cpp:627:24: error: ‘const struct llvm::StackMaps::FunctionInfo’ has no member named ‘second’
OS.emitIntValue(FR.second.RecordCount, 8);
^~~~~~Comment Actions 6a684dbc4433a33e5f94fb15c9e378a2408021e0 should have fixed the build for GCC 7.5.0. GCC 7.3.0 has many other issues as the commit message said... Comment Actions I can confirm that 6a684dbc4433a33e5f94fb15c9e378a2408021e0 fixed the build with GCC 7.5.0. Thanks. Comment Actions We should have a buildbot that uses GCC 7.5.0 to check build issues before they are landed. Comment Actions Yes, but the reality is that we don't have one. I think we may have a GCC 8 bot. Yes. I filed https://github.com/llvm/llvm-project/issues/64117 |