[PATCH] D103315: Add interface to order inlining

[Kazu Hirata via Phabricator via llvm-commits]

kazu added a comment.

Thanks for the update.  I'd generalize the code around `Call.erase` a little bit more to accommodate other underlying data structures.  See the comments below for details.



================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:67
 #include <cassert>
+#include <deque>
 #include <functional>
----------------
Remove this if you use `SmallVector`.  See below.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:690
+
+  virtual iterator erase(const_iterator B, const_iterator E) = 0;
+
----------------
I would change this:

```
  virtual void erase_if(function_ref<bool (CallBase *)> Pred) = 0;
```

where you would erase all elements satisfying `Pred` -- somewhat like `erase_if` in `llvm/include/llvm/ADT/STLExtras.h`.

I'm suggesting this because you need to heapify the contents after erasing elements if your underlying structure is a priority queue for example.  `std::remove_if` removes items while maintaining the iteration order of surviving elements, but this property is specific to `DefaultInlineOrder`.  Other inline orders may not be compatible with this property.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:692-693
+
+  virtual iterator begin() = 0;
+  virtual iterator end() = 0;
+
----------------
You shouldn't need these functions once you implement `erase_if`.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:698
+
+template <typename T, typename Container = std::deque<T>>
+class DefaultInlineOrder : public InlineOrder<T, Container> {
----------------
If we never add things to the front, I would stick to `SmallVector` and keep an index `I` to point to the first valid element just like the original code.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:714
+
+  iterator erase(const_iterator CB, const_iterator CE) override {
+    return Calls.erase(CB, CE);
----------------
Change to `erase_if` accordingly.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:718-719
+
+  iterator begin() override { return Calls.begin(); }
+  iterator end() override { return Calls.end(); }
+
----------------
Likewise, you shouldn't need these.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:932-937
           Calls.erase(
-              std::remove_if(Calls.begin() + I + 1, Calls.end(),
+              std::remove_if(Calls.begin(), Calls.end(),
                              [&](const std::pair<CallBase *, int> &Call) {
                                return Call.first->getCaller() == &Callee;
                              }),
               Calls.end());
----------------
Change this to:

```
Calls.erase_if([&](CallBase *Call) { return Call->getCaller() == &Callee; });
```

This way, you shouldn't need `begin` or `end`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103315