[PATCH] D84951: [LV] Try to sink users recursively for first-order recurrences.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

In D84951#2760659 <https://reviews.llvm.org/D84951#2760659>, @fhahn wrote:

> Rebased, update to also avoid sinking instructions that may read from memory, as we cannot move them over ones with side-effects, move sort to isFirstOrderRecurrence.
>
> In D84951#2753861 <https://reviews.llvm.org/D84951#2753861>, @Ayal wrote:
>
>> How about making SinkAfter a MapVector instead of DenseMap (to have its iteration order match insertion order) and insert interdependent sink candidates in the desired order?
>> How about traversing all insns from phi to previous, in order, checking if any is using the phi or a prior insn that was sunk? E.g., by maintaining the set of their users.
>> This might be clearer, and avoid the sorting.
>
> I'm not sure if it is possible to maintain the order by dominance in the map as we go along, because we an instruction could be used by multiple others and we may visit them in reverse dominance order (relative to each other), so we would miss the fact we need to 'fix' the order of the first instruction. I think it is also difficult & expensive to change the order of multiple entries in the map vector.
>
> I added a few more interesting scenarios as tests in 68d52f0dbe2e <https://reviews.llvm.org/rG68d52f0dbe2ef1fd36850c2a637d6db70f509b32> and c62f984814c4 <https://reviews.llvm.org/rGc62f984814c4994911dee75bad28cb7346ca3c07>.
>
> I *think* however it should be enough to sort only sort the instructions to sink per FOR phi, as we currently allow sinking for each instruction only once. That should certainly be better, as we have less to sort overall and everything is contained in IVDescriptor.cpp. The new code already maintains a vector of instructions to sink, so the sort fits in quite nicely. WDYT?

The instructions to sink reside inside the header block in the desired order. It should be possible to traverse them, once, from the FOR phi to the terminator or Previous, w/o sorting nor changing order, by maintaining the set of phi's (direct and indirect) users. When encountering such a user during the traversal, check if it is sunkable, append it to be moved after the last sunken user (or after Previous if this is the first user being sunk), and add its users to the set - those that belong to the header block, others need to be dominated by (and distinct from) Previous.
Perhaps this would also help support sinking memory instructions in the future.
Sounds reasonable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84951