[PATCH] D41766: [MachineCombiner] Add check for optimal pattern order.

Thu Jul 6 08:02:41 PDT 2023

madhur13490 added a comment.

In D41766#4474557 <https://reviews.llvm.org/D41766#4474557>, @fhahn wrote:

>> I have a high-level question for this patch. I understand that the verify function aims to verify patterns to be present in certain order but insertion is not guaranteed to be in the same order.
>>
>> Does it make sense to have this verification in the first place?
>
> The machine combiner applies the pattern in order, so the goal is to ensure that the most profitable patterns are processed first. Did you encounter cases where it wasn't possible to fix the insertion order?

So far not but the I have one case where we had to find it meticulously to fix the order and it is time-consuming each time. It is not possible to make sure that we always do it. EXPENSIVE_CHECK builds are not very well tested as I couldn't see a build bot for aarch64 platform. (I might be wrong if added recently).

If the goal is to make sure we apply patterns in certain order then we should sort them based on latency. Verifying without making sure the insertion order is a risk and we have more than one cases. Moreover, at the insertion point we have no clue if it is inserting in the required order.

Please let me know if I am missing something here.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D41766