[PATCH] D135137: [AggressiveInstCombine] Load merge the reverse load pattern and mixed load sizes.

Fri Oct 14 07:37:59 PDT 2022

spatel added a comment.

In D135137#3858404 <https://reviews.llvm.org/D135137#3858404>, @bipmis wrote:

> In D135137#3856034 <https://reviews.llvm.org/D135137#3856034>, @spatel wrote:
>
>> This improves the pattern-matching, but it's still not complete, right? Ie, we should have a PhaseOrdering test with all 24 (4!) combinations of four i8-loads spliced into a 32-bit value, so we know the pattern is matched no matter what order the values are put back together with 'or'. We can probably rely on instcombine to canonicalize half of those patterns, but I'm not sure there's anything else there to reduce the matching space.
>
> Right but won't these cover the most commonly seen patterns in the real application scenario, if not all considering it belongs to a pattern which generates a wider load.

In dealing with the related case of trying to match bswap patterns, we've found that eventually we will see every possible pattern in source code somewhere. 
I don't disagree that the in-order/reverse are the most likely (and I won't hold up improvements from this patch), but I'd like to have tests in place that acknowledge that we are aware of the limitations of the current implementation. For users, having inconsistent optimization can be almost as frustrating as having no optimization.

> InstCombine can possibly canonicalize the or-chain of loads in an ascending/descending order of load indexes. The fact that it is called multiple times should get us the pattern expected by AggressiveInstCombine. However, I am not sure if this is the right thing to do.

I expect that we'll end up using cooperation between "reassociate", "instcombine", and "aggressive-instcombine" to get the optimal loads. That's why I suggest PhaseOrdering tests - it's not clear which pass will be responsible for canonicalizing everything, but we do want to make sure that a complete trip through the optimizer will catch all patterns. There's also an open question of when and how often to invoke aggressive-instcombine (for example, it's only enabled at -O3 currently).

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

https://reviews.llvm.org/D135137