[PATCH] D125588: [MachineCombiner] Improve MachineCombiner's cost model

[Guozhi Wei via Phabricator via llvm-commits]

Carrot added a comment.

In D125588#3610936 <https://reviews.llvm.org/D125588#3610936>, @dmgreen wrote:

> I think - the old costmodel was not great, but the new code is still incorrect for some cases. For the MADD case we don't handle the differing latencies of each of the input operand. (The latency for the mul operand might be 3, but for the add operand is 1. The cost just always uses "depth" + 3). There may also be differences when inserting multiple instructions too. The old model was `lastinstrdepth + sum(latencies)`. The new one is `firstinstrdepth + sum(latencies)`, which I feel may not be more accurate if the instructions in `InsInstrs` have dependencies that are not in InsInstrs.

You are right about the comparison. It is also partly mentioned in the comments of function getLatenciesForInstrSequences.

  /// Estimate the latency of the new and original instruction sequence by summing
  /// up the latencies of the inserted and deleted instructions. This assumes
  /// that the inserted and deleted instructions are dependent instruction chains,
  /// which might not hold in all cases.

So the new model `firstinstrdepth + sum(latencies)` is not correct if InsInstrs is not a single dependence chain. But it is still correct in many cases.
The old model `lastinstrdepth + sum(latencies)` is always wrong for a multiple instruction sequence InsInstrs, because instructions in the range [0..n-2] are counted in both firstinstrdepth and sum(latencies).

> As a concrete way forward - can you take the isCoalescableCopy and split it into a separate patch. It can also change the `if (!MI->isCopy())    return false;` to `if (!MI->isCopy())    return MI->isTransient();` and maybe rename the function to isTransient. That should be fine to go separately, I believe, and the isCoalescableCopy makes a nice change.

I will do.

> For the rest we may need something that more accurately calculates the final depth for instructions with forwarding like the MADD case.

Agree.

thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125588