[PATCH] D113424: ext-tsp basic block layout

Thu Oct 12 11:18:19 PDT 2023

spupyrev added inline comments.

================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:404-405
+    Result.insert(Result.end(), Begin1, End1);
+    Result.insert(Result.end(), Begin2, End2);
+    Result.insert(Result.end(), Begin3, End3);
+    return Result;
----------------
kosarev wrote:
> spupyrev wrote:
> > kosarev wrote:
> > > spupyrev wrote:
> > > > kosarev wrote:
> > > > > This seems to fail on expensive checks, see <https://github.com/llvm/llvm-project/issues/68594>.
> > > > > 
> > > > > Do `{Begin,End}{2,3}` really form valid ranges when default to `BlockIter()` in the constructor?
> > > > Can you teach me to reproduce the failure? I'm building with "-DCMAKE_BUILD_TYPE=Debug -DLLVM_ENABLE_ASSERTIONS=ON" and run "ninja check-all" but still don't see it
> > > It fails on libc++'s consistency checks, which get enabled automatically on builds with LLVM's expensive checks turned on, `-DLLVM_ENABLE_EXPENSIVE_CHECKS=ON`.
> > hmm. I still cannot reproduce this. Here is my command:
> > ```
> > cmake \
> >     -G Ninja \
> >     -DCMAKE_ASM_COMPILER=$MYCLANG \
> >     -DCMAKE_ASM_COMPILER_ID=Clang \
> >     -DCMAKE_BUILD_TYPE=Debug \
> >     -DCMAKE_CXX_COMPILER=$MYCLANG++ \
> >     -DCMAKE_C_COMPILER=$MYCLANG \
> >     -DLLVM_TARGETS_TO_BUILD="X86;AArch64" \
> >     -DLLVM_DEFAULT_TARGET_TRIPLE=x86_64-redhat-linux-gnu \
> >     -DLLVM_ENABLE_ASSERTIONS=ON \
> >     -DLLVM_ENABLE_LLD=ON \
> >     -DLLVM_ENABLE_PROJECTS="clang;lld;bolt" \
> >     -DCMAKE_EXE_LINKER_FLAGS="-L /usr/lib64" \
> >     -DLLVM_ENABLE_EXPENSIVE_CHECKS=ON \
> >     ../llvm
> > 
> >      ninja check-all
> > ```
> > Is something else missing?
> Looks OK to me. I'd make sure the code is rebuilt from scratch and `_GLIBCXX_DEBUG` is defined, then look into why libc++ doesn't do the checks. I'm attaching a backtrace of one of the failures, in case it may be of any help.
> 
> That regardless, I think the nature of the problem is very clear, which is that when the Begin1/2 and End1/2 iterators are singular, they should not be used to form any ranges. Once we have a fix, I'd be happy to test it for you.
> 
> ```
> [ RUN      ] CodeLayout.ThreeFunctions
> /usr/include/c++/11/debug/vector:617:
> In function:
>     std::debug::vector<_Tp, _Allocator>::iterator std::debug::vector<_Tp, 
>     _Allocator>::insert(std::debug::vector<_Tp, _Allocator>::const_iterator, 
>     _InputIterator, _InputIterator) [with _InputIterator = 
>     gnu_debug::_Safe_iterator<gnu_cxx::normal_iterator<{anonymous}::NodeT* 
>     const*, std::vector<{anonymous}::NodeT*, 
>     std::allocator<{anonymous}::NodeT*> > >, std::
>     debug::vector<{anonymous}::NodeT*>, std::random_access_iterator_tag>; 
>     <template-parameter-2-2> = void; _Tp = {anonymous}::NodeT*; _Allocator = 
>     std::allocator<{anonymous}::NodeT*>; std::debug::vector<_Tp, 
>     _Allocator>::iterator = std::
>     debug::vector<{anonymous}::NodeT*>::iterator; std::debug::vector<_Tp, 
>     _Allocator>::const_iterator = std::
>     debug::vector<{anonymous}::NodeT*>::const_iterator]
> 
> Error: function requires a valid iterator range [first, last).
> 
> Objects involved in the operation:
>     iterator "first" @ 0x7ffe7dbd1c40 {
>       state = singular;
>     }
>     iterator "last" @ 0x7ffe7dbd1c70 {
>       state = singular;
>     }
>  #0 0x00007f5c10bc3b02 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /home/kosarev/labs/llvm-project/llvm/lib/Support/Unix/Signals.inc:723:22
>  #1 0x00007f5c10bc3f1e PrintStackTraceSignalHandler(void*) /home/kosarev/labs/llvm-project/llvm/lib/Support/Unix/Signals.inc:798:1
>  #2 0x00007f5c10bc1363 llvm::sys::RunSignalHandlers() /home/kosarev/labs/llvm-project/llvm/lib/Support/Signals.cpp:105:20
>  #3 0x00007f5c10bc339a SignalHandler(int) /home/kosarev/labs/llvm-project/llvm/lib/Support/Unix/Signals.inc:413:1
>  #4 0x00007f5c10447520 (/lib/x86_64-linux-gnu/libc.so.6+0x42520)
>  #5 0x00007f5c1049b9fc __pthread_kill_implementation ./nptl/./nptl/pthread_kill.c:44:76
>  #6 0x00007f5c1049b9fc __pthread_kill_internal ./nptl/./nptl/pthread_kill.c:78:10
>  #7 0x00007f5c1049b9fc pthread_kill ./nptl/./nptl/pthread_kill.c:89:10
>  #8 0x00007f5c10447476 gsignal ./signal/../sysdeps/posix/raise.c:27:6
>  #9 0x00007f5c1042d7f3 abort ./stdlib/./stdlib/abort.c:81:7
> #10 0x00007f5c106d21fb std::__throw_bad_exception() (/lib/x86_64-linux-gnu/libstdc++.so.6+0xa51fb)
> #11 0x00007f5c12dd8740 __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<(anonymous namespace)::NodeT**, std::__cxx1998::vector<(anonymous namespace)::NodeT*, std::allocator<(anonymous namespace)::NodeT*> > >, std::__debug::vector<(anonymous namespace)::NodeT*, std::allocator<(anonymous namespace)::NodeT*> >, std::random_access_iterator_tag> std::__debug::vector<(anonymous namespace)::NodeT*, std::allocator<(anonymous namespace)::NodeT*> >::insert<__gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<(anonymous namespace)::NodeT* const*, std::__cxx1998::vector<(anonymous namespace)::NodeT*, std::allocator<(anonymous namespace)::NodeT*> > >, std::__debug::vector<(anonymous namespace)::NodeT*, std::allocator<(anonymous namespace)::NodeT*> >, std::random_access_iterator_tag>, void>(__gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<(anonymous namespace)::NodeT* const*, std::__cxx1998::vector<(anonymous namespace)::NodeT*, std::allocator<(anonymous namespace)::NodeT*> > >, std::__debug::vector<(anonymous namespace)::NodeT*, std::allocator<(anonymous namespace)::NodeT*> >, std::random_access_iterator_tag>, __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<(anonymous namespace)::NodeT* const*, std::__cxx1998::vector<(anonymous namespace)::NodeT*, std::allocator<(anonymous namespace)::NodeT*> > >, std::__debug::vector<(anonymous namespace)::NodeT*, std::allocator<(anonymous namespace)::NodeT*> >, std::random_access_iterator_tag>, __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<(anonymous namespace)::NodeT* const*, std::__cxx1998::vector<(anonymous namespace)::NodeT*, std::allocator<(anonymous namespace)::NodeT*> > >, std::__debug::vector<(anonymous namespace)::NodeT*, std::allocator<(anonymous namespace)::NodeT*> >, std::random_access_iterator_tag>) /usr/include/c++/11/debug/vector:617:4
> #12 0x00007f5c12dcee70 (anonymous namespace)::MergedChain::getNodes() const /home/kosarev/labs/llvm-project/llvm/lib/Transforms/Utils/CodeLayout.cpp:504:18
> #13 0x00007f5c12dd5141 (anonymous namespace)::CDSortImpl::mergeChains((anonymous namespace)::ChainT*, (anonymous namespace)::ChainT*, unsigned long, (anonymous namespace)::MergeTypeT) /home/kosarev/labs/llvm-project/llvm/lib/Transforms/Utils/CodeLayout.cpp:1288:16
> #14 0x00007f5c12dd4211 (anonymous namespace)::CDSortImpl::mergeChainPairs() /home/kosarev/labs/llvm-project/llvm/lib/Transforms/Utils/CodeLayout.cpp:1144:50
> #15 0x00007f5c12dd302c (anonymous namespace)::CDSortImpl::run() /home/kosarev/labs/llvm-project/llvm/lib/Transforms/Utils/CodeLayout.cpp:1008:5
> #16 0x00007f5c12dd615c llvm::codelayout::computeCacheDirectedLayout(llvm::codelayout::CDSortConfig const&, llvm::ArrayRef<unsigned long>, llvm::ArrayRef<unsigned long>, llvm::ArrayRef<llvm::codelayout::EdgeCount>, llvm::ArrayRef<unsigned long>) /home/kosarev/labs/llvm-project/llvm/lib/Transforms/Utils/CodeLayout.cpp:1435:3
> #17 0x00007f5c12dd6333 llvm::codelayout::computeCacheDirectedLayout(llvm::ArrayRef<unsigned long>, llvm::ArrayRef<unsigned long>, llvm::ArrayRef<llvm::codelayout::EdgeCount>, llvm::ArrayRef<unsigned long>) /home/kosarev/labs/llvm-project/llvm/lib/Transforms/Utils/CodeLayout.cpp:1453:48
> #18 0x00005638fc0abb97 (anonymous namespace)::CodeLayout_ThreeFunctions_Test::TestBody() /home/kosarev/labs/llvm-project/llvm/unittests/Transforms/Utils/CodeLayoutTest.cpp:18:78
> #19 0x00007f5c13e2ef1a void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /home/kosarev/labs/llvm-project/third-party/unittest/googletest/src/gtest.cc:2612:28
> #20 0x00007f5c13e1f20f void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /home/kosarev/labs/llvm-project/third-party/unittest/googletest/src/gtest.cc:2667:75
> #21 0x00007f5c13df2bca testing::Test::Run() /home/kosarev/labs/llvm-project/third-party/unittest/googletest/src/gtest.cc:2694:30
> #22 0x00007f5c13df350b testing::TestInfo::Run() /home/kosarev/labs/llvm-project/third-party/unittest/googletest/src/gtest.cc:2839:3
> #23 0x00007f5c13df3eea testing::TestSuite::Run() /home/kosarev/labs/llvm-project/third-party/unittest/googletest/src/gtest.cc:3017:35
> #24 0x00007f5c13e01306 testing::internal::UnitTestImpl::RunAllTests() /home/kosarev/labs/llvm-project/third-party/unittest/googletest/src/gtest.cc:5921:41
> #25 0x00007f5c13e315fd bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/kosarev/labs/llvm-project/third-party/unittest/googletest/src/gtest.cc:2614:1
> #26 0x00007f5c13e20b7a bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/kosarev/labs/llvm-project/third-party/unittest/googletest/src/gtest.cc:2667:75
> #27 0x00007f5c13dffc25 testing::UnitTest::Run() /home/kosarev/labs/llvm-project/third-party/unittest/googletest/src/gtest.cc:5487:14
> #28 0x00007f5c13efde7f RUN_ALL_TESTS() /home/kosarev/labs/llvm-project/third-party/unittest/googletest/include/gtest/gtest.h:2317:80
> #29 0x00007f5c13efdd61 main /home/kosarev/labs/llvm-project/third-party/unittest/UnitTestMain/TestMain.cpp:55:24
> #30 0x00007f5c1042ed90 __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:58:16
> #31 0x00007f5c1042ee40 call_init ./csu/../csu/libc-start.c:128:20
> #32 0x00007f5c1042ee40 __libc_start_main ./csu/../csu/libc-start.c:379:5
> #33 0x00005638fc0423e5 _start (/home/kosarev/labs/llvm-project/build/debug+expensive_checks/unittests/Transforms/Utils/./UtilsTests+0x1f3e5)
> ```
Thanks for such a detailed analysis and for teaching me about singular iterators.
https://github.com/llvm/llvm-project/pull/68917

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113424/new/

https://reviews.llvm.org/D113424