[PATCH] D134056: [ModuleInliner] Move InlinePriority and its derived classes to InlineOrder.cpp (NFC)

Fri Oct 13 06:26:47 PDT 2023

taolq added inline comments.

================
Comment at: llvm/lib/Analysis/InlineOrder.cpp:124
+    while (PriorityPtr->updateAndCheckDecreased(Heap.front())) {
+      std::pop_heap(Heap.begin(), Heap.end(), isLess);
+      std::push_heap(Heap.begin(), Heap.end(), isLess);
----------------
kosarev wrote:
> taolq wrote:
> > taolq wrote:
> > > kosarev wrote:
> > > > It looks something is not quite right about how this heap is formed. An `assert(std::is_heap(Heap.begin(), Heap.end(), isLess));` added before that line would fail on `Transforms/Inline/nested-inline.ll`. Can anyone take a look, please?
> > > > 
> > > > Originally caught on a build with libc++'s consistency checks enabled, <https://github.com/llvm/llvm-project/issues/68594>.
> > > Thanks for the information. I will take a look.
> > I just figure it out. The `assert` you added failed because the Heap is updated lazily. After line 123 `updateAndCheckDecreased`, the priority has been changed, but the Heap has not been resorted yet.
> > So `assert(std::is_heap(Heap.begin(), Heap.end(), isLess));` should be appended after std::push_heap.
> > And `Transforms/Inline/nested-inline.ll` would pass with that change.
> I think `std:pop_heap()` requires the range to be a valid heap by the moment of the call; we should not rely on `std::push_heap()` fixing the heap.
> 
> Here's the backtrace:
> ```
> ******************** TEST 'LLVM :: Transforms/Inline/nested-inline.ll' FAILED ********************
> Exit Code: 2
> 
> Command Output (stderr):
> --
> RUN: at line 1: /home/kosarev/labs/llvm-project/build/debug+expensive_checks/bin/opt < /home/kosarev/labs/llvm-project/llvm/test/Transforms/Inline/nested-inline.ll -passes=inline -S | /home/kosarev/labs/llvm-project/build/debug+expensive_checks/bin/FileCheck /home/kosarev/labs/llvm-project/llvm/test/Transforms/Inline/nested-inline.ll
> + /home/kosarev/labs/llvm-project/build/debug+expensive_checks/bin/opt -passes=inline -S
> + /home/kosarev/labs/llvm-project/build/debug+expensive_checks/bin/FileCheck /home/kosarev/labs/llvm-project/llvm/test/Transforms/Inline/nested-inline.ll
> RUN: at line 2: /home/kosarev/labs/llvm-project/build/debug+expensive_checks/bin/opt < /home/kosarev/labs/llvm-project/llvm/test/Transforms/Inline/nested-inline.ll -passes='cgscc(inline)' -S | /home/kosarev/labs/llvm-project/build/debug+expensive_checks/bin/FileCheck /home/kosarev/labs/llvm-project/llvm/test/Transforms/Inline/nested-inline.ll
> + /home/kosarev/labs/llvm-project/build/debug+expensive_checks/bin/opt '-passes=cgscc(inline)' -S
> + /home/kosarev/labs/llvm-project/build/debug+expensive_checks/bin/FileCheck /home/kosarev/labs/llvm-project/llvm/test/Transforms/Inline/nested-inline.ll
> RUN: at line 3: /home/kosarev/labs/llvm-project/build/debug+expensive_checks/bin/opt < /home/kosarev/labs/llvm-project/llvm/test/Transforms/Inline/nested-inline.ll -passes='module-inline' -S | /home/kosarev/labs/llvm-project/build/debug+expensive_checks/bin/FileCheck /home/kosarev/labs/llvm-project/llvm/test/Transforms/Inline/nested-inline.ll
> + /home/kosarev/labs/llvm-project/build/debug+expensive_checks/bin/opt -passes=module-inline -S
> + /home/kosarev/labs/llvm-project/build/debug+expensive_checks/bin/FileCheck /home/kosarev/labs/llvm-project/llvm/test/Transforms/Inline/nested-inline.ll
> /usr/include/c++/11/bits/stl_heap.h:325:
> In function:
>     void std::pop_heap(_RAIter, _RAIter, _Compare) [with _RAIter = 
>     llvm::CallBase**; _Compare = std::function<bool(const llvm::CallBase*, 
>     const llvm::CallBase*)>]
> 
> Error: elements in iterator range [first, last) do not form a heap with 
> respect to the predicate __comp.
> 
> Objects involved in the operation:
>     iterator "first" @ 0x7ffe489f0268 {
>     }
>     iterator "last" @ 0x7ffe489f0260 {
>     }
> PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
> Stack dump:
> 0.	Program arguments: /home/kosarev/labs/llvm-project/build/debug+expensive_checks/bin/opt -passes=module-inline -S
>  #0 0x00007fd0a21586c8 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /home/kosarev/labs/llvm-project/llvm/lib/Support/Unix/Signals.inc:723:22
>  #1 0x00007fd0a2158ae4 PrintStackTraceSignalHandler(void*) /home/kosarev/labs/llvm-project/llvm/lib/Support/Unix/Signals.inc:798:1
>  #2 0x00007fd0a2155f29 llvm::sys::RunSignalHandlers() /home/kosarev/labs/llvm-project/llvm/lib/Support/Signals.cpp:105:20
>  #3 0x00007fd0a2157f60 SignalHandler(int) /home/kosarev/labs/llvm-project/llvm/lib/Support/Unix/Signals.inc:413:1
>  #4 0x00007fd0a19dd520 (/lib/x86_64-linux-gnu/libc.so.6+0x42520)
>  #5 0x00007fd0a1a319fc __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
>  #6 0x00007fd0a1a319fc __pthread_kill_internal ./nptl/pthread_kill.c:78:10
>  #7 0x00007fd0a1a319fc pthread_kill ./nptl/pthread_kill.c:89:10
>  #8 0x00007fd0a19dd476 gsignal ./signal/../sysdeps/posix/raise.c:27:6
>  #9 0x00007fd0a19c37f3 abort ./stdlib/abort.c:81:7
> #10 0x00007fd0a1c681fb std::__throw_bad_exception() (/lib/x86_64-linux-gnu/libstdc++.so.6+0xa51fb)
> #11 0x00007fd0a35e2228 void std::pop_heap<llvm::CallBase**, std::function<bool (llvm::CallBase const*, llvm::CallBase const*)>>(llvm::CallBase**, llvm::CallBase**, std::function<bool (llvm::CallBase const*, llvm::CallBase const*)>) /usr/include/c++/11/bits/stl_heap.h:327:18
> #12 0x00007fd0a35da8b5 (anonymous namespace)::PriorityInlineOrder<(anonymous namespace)::SizePriority>::adjust() /home/kosarev/labs/llvm-project/llvm/lib/Analysis/InlineOrder.cpp:227:20
> #13 0x00007fd0a35d9f98 (anonymous namespace)::PriorityInlineOrder<(anonymous namespace)::SizePriority>::pop() /home/kosarev/labs/llvm-project/llvm/lib/Analysis/InlineOrder.cpp:256:30
> #14 0x00007fd0a85337d0 llvm::ModuleInlinerPass::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) /home/kosarev/labs/llvm-project/llvm/lib/Transforms/IPO/ModuleInliner.cpp:180:24
> #15 0x00007fd0a90fb66d llvm::detail::PassModel<llvm::Module, llvm::ModuleInlinerPass, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) /home/kosarev/labs/llvm-project/llvm/include/llvm/IR/PassManagerInternal.h:90:3
> #16 0x00007fd0a294e171 llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) /home/kosarev/labs/llvm-project/llvm/include/llvm/IR/PassManager.h:521:20
> #17 0x000055c2085d4371 llvm::runPassPipeline(llvm::StringRef, llvm::Module&, llvm::TargetMachine*, llvm::TargetLibraryInfoImpl*, llvm::ToolOutputFile*, llvm::ToolOutputFile*, llvm::ToolOutputFile*, llvm::StringRef, llvm::ArrayRef<llvm::PassPlugin>, llvm::opt_tool::OutputKind, llvm::opt_tool::VerifierKind, bool, bool, bool, bool, bool, bool, bool) /home/kosarev/labs/llvm-project/llvm/tools/opt/NewPMDriver.cpp:527:10
> #18 0x000055c208605385 main /home/kosarev/labs/llvm-project/llvm/tools/opt/opt.cpp:698:27
> #19 0x00007fd0a19c4d90 __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:58:16
> #20 0x00007fd0a19c4e40 call_init ./csu/../csu/libc-start.c:128:20
> #21 0x00007fd0a19c4e40 __libc_start_main ./csu/../csu/libc-start.c:379:5
> #22 0x000055c2085d1605 _start (/home/kosarev/labs/llvm-project/build/debug+expensive_checks/bin/opt+0x1c605)
> FileCheck error: '<stdin>' is empty.
> FileCheck command line:  /home/kosarev/labs/llvm-project/build/debug+expensive_checks/bin/FileCheck /home/kosarev/labs/llvm-project/llvm/test/Transforms/Inline/nested-inline.ll
> ```
It makes sense. I will fix it asap.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134056