[PATCH] D133584: [DAGCombiner] [AMDGPU] Allow vector loads in MatchLoadCombine

Mon Sep 19 11:09:15 PDT 2022

spatel added a comment.

In D133584#3800232 <https://reviews.llvm.org/D133584#3800232>, @jrbyrnes wrote:

> In D133584#3799669 <https://reviews.llvm.org/D133584#3799669>, @spatel wrote:
>
>> Both SLP and VectorCombine should try to make patterns like this better in IR, so there might be some target cost/legality checks that need adjusting.
>> There's also an in-progress patch for -aggressive-instcombine that could be relevant:
>> D127392 <https://reviews.llvm.org/D127392>
>>
>> Would it be better to transform this before codegen?
>> https://alive2.llvm.org/ce/z/uyxHSW
>
> Hi, thanks for your comment! The reason I tagged you is because you seem to be involved in the most closely related issues to the one here (D67841 <https://reviews.llvm.org/D67841>, https://bugs.llvm.org/show_bug.cgi?id=42708). It seems the conclusion is to have vectorization passes (and optimization passes in general) leave LoadCombine patterns untouched, and resolve it in the backend, no? That was the logic I used for the design here.
>
> On the other hand, it seems D127392 <https://reviews.llvm.org/D127392> is using the opposite design approach. Is the current approach to do load combining in optimizer?

LLVM has gone back and forth on this. There was a general load combine pass for IR, but it was removed because it interfered with other transforms in IR. So we started hacking away at codegen instead, but there are programs where doing the transform in codegen is too late to get the optimal results. So we have some limited transforms in the vectorization passes, and now we're trying to reintroduce load combining as a canonicalization (but in very limited cases and gated by target-specific legality checks).

> At a glance, D127392 <https://reviews.llvm.org/D127392> will not address the issue identified here because it does not handle vector loads.

Right - getting that to work correctly on the most basic integer load patterns is the first step, but we could enhance the transform for more cases (hopefully without too much work).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133584