[ModuleInliner] use std::make_heap to construct the heap after element updated

[taoliq]

After updateAndCheckDecreased, some element may be updated and the range(Heap.begin(), Heap.end()) is not a valid heap.
Use std::make_heap to make sure the heap is valid.

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Author: Liqiang TAO (taoliq)

Changes

After updateAndCheckDecreased, some element may be updated and the range(Heap.begin(), Heap.end()) is not a valid heap.
Use std::make_heap to make sure the heap is valid.

---

Full diff: https://github.com/llvm/llvm-project/pull/69206.diff

1 Files Affected:

• (modified) llvm/lib/Analysis/InlineOrder.cpp (+4-5)

diff --git a/llvm/lib/Analysis/InlineOrder.cpp b/llvm/lib/Analysis/InlineOrder.cpp
index 503880e3e8f0e93..98dea4846ec1103 100644
--- a/llvm/lib/Analysis/InlineOrder.cpp
+++ b/llvm/lib/Analysis/InlineOrder.cpp
@@ -219,13 +219,12 @@ class PriorityInlineOrder : public InlineOrder<std::pair<CallBase *, int>> {
   // growth from prior inlining into the callee. This method is used to lazily
   // update the desirability of a call site if it's decreasing. It is only
   // called on pop() or front(), not every time the desirability changes. When
-  // the desirability of the front call site decreases, an updated one would be
-  // pushed right back into the heap. For simplicity, those cases where
-  // the desirability of a call site increases are ignored here.
+  // the desirability of the front call site decreases, the heap is re-constructed
+  // by std::make_heap. For simplicity, those cases where the desirability of
+  // a call site increases are ignored here.
   void adjust() {
     while (updateAndCheckDecreased(Heap.front())) {
-      std::pop_heap(Heap.begin(), Heap.end(), isLess);
-      std::push_heap(Heap.begin(), Heap.end(), isLess);
+      std::make_heap(Heap.begin(), Heap.end(), isLess);
     }
   }
 

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff f41ec27f7eba34548a280a4a4d7de2ef32837210 8ba49949cfbeda7ad9bccd7231f8154c5281603a -- llvm/lib/Analysis/InlineOrder.cpp

View the diff from clang-format here.

diff --git a/llvm/lib/Analysis/InlineOrder.cpp b/llvm/lib/Analysis/InlineOrder.cpp
index 98dea4846..5280f100e 100644
--- a/llvm/lib/Analysis/InlineOrder.cpp
+++ b/llvm/lib/Analysis/InlineOrder.cpp
@@ -219,9 +219,9 @@ class PriorityInlineOrder : public InlineOrder<std::pair<CallBase *, int>> {
   // growth from prior inlining into the callee. This method is used to lazily
   // update the desirability of a call site if it's decreasing. It is only
   // called on pop() or front(), not every time the desirability changes. When
-  // the desirability of the front call site decreases, the heap is re-constructed
-  // by std::make_heap. For simplicity, those cases where the desirability of
-  // a call site increases are ignored here.
+  // the desirability of the front call site decreases, the heap is
+  // re-constructed by std::make_heap. For simplicity, those cases where the
+  // desirability of a call site increases are ignored here.
   void adjust() {
     while (updateAndCheckDecreased(Heap.front())) {
       std::make_heap(Heap.begin(), Heap.end(), isLess);

[kazutakahirata]

Hmm. std::make_heap in the loop would be expensive. We would be looking at all elements in the queue every time we pop one item. Let me take some time and try to understand the underlying issue better.

[kazutakahirata]

@taoliq, the following seems to work even with the expensive checks enabled. I'll post this as separate PR myself:

    std::pop_heap(Heap.begin(), Heap.end(), isLess);
    while (updateAndCheckDecreased(Heap.back())) {
      std::push_heap(Heap.begin(), Heap.end(), isLess);
      std::pop_heap(Heap.begin(), Heap.end(), isLess);
    }
    std::push_heap(Heap.begin(), Heap.end(), isLess);

Although pop_heap() simply swaps the first and last element of the heap, the libstdc++ version (and possibly the C++ standard) requires that the entire range be a valid heap when expensive checks are enabled.

The code snippet above:

• swaps the first and last element while Heap is a valid heap,
• calls updateAndCheckDecreased on the last element, which is outside the valid heap, and
• put the last element back into the heap.

In other words, we use the last element as a safe work area while we are updating its priority.

We could reduce the number of calls to updateAndCheckDecreased in certain situations, but let's take care of the crash first.

[kazutakahirata]

I've posted #69251 as an alternative fix.

[taoliq]

I've posted #69251 as an alternative fix.

Thanks Kazu. I like your fix, so I'd like to close this one.