[PATCH] D97382: NFC: Migrate PartialInlining to work on InstructionCost

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added inline comments.


================
Comment at: llvm/include/llvm/Support/InstructionCost.h:205-209
+  InstructionCost map(const std::function<CostType(const CostType &)> &F) {
+    if (isValid())
+      return InstructionCost(F(*getValue()));
+    return getInvalid();
+  }
----------------
sdesmalen wrote:
> ctetreau wrote:
> > ctetreau wrote:
> > > Having the function type be a template parameter (as it is defined in Optional.h) is preferred because lambda types can be directly used without the indirection of assigning them to a std::function. It also avoids having to `#include <functional>`
> > also, I believe map can be const.
> > Having the function type be a template parameter (as it is defined in Optional.h) is preferred because lambda types can be directly used without the indirection of assigning them to a std::function. It also avoids having to #include <functional>
> Okay, I wasn't sure if it was better to limit the signature of Function by specifying it would take/return a CostType by using std::function. Any issues resulting from the lambda return the wrong value type probably show up when calling the constructor of InstructionCost.
Yeah, the function object has to take one argument that coerces to CostType, and return some type that coerces to InstructionCost, so it should be safe. I guess it would be nice to constrain the signature to prevent people from thinking they need to return InstructionCost and invoking the copy constructor for no reason, but this version is safer to call in a hot inner loop which I think most C++ programmers would prefer.


================
Comment at: llvm/lib/Transforms/IPO/PartialInlining.cpp:1363
+  auto OutliningCosts = computeOutliningCosts(Cloner);
+  if (!std::get<0>(OutliningCosts).isValid() ||
+      !std::get<1>(OutliningCosts).isValid())
----------------
this looks like a behavioral change. Please just assert that the costs are valid.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97382