[PATCH] D69228: [LV] Generalize conditions for sinking instrs for first order recurrences.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D69228#1737857 <https://reviews.llvm.org/D69228#1737857>, @Ayal wrote:

> As revert eaff300 <https://reviews.llvm.org/rGeaff3004019f97c64c88ab76da6b25106b659b30> shows, more care must be taken to avoid having one FOR (First Order Recurring) phi sink an instruction that another FOR phi needs to keep in place. Namely, any `Previous` instruction that feeds a FOR phi across the backarc must not be allowed to sink, because that may break its dominance on other instructions who use the FOR phi. The original check against `SinkAfter.count(Previous)) // Cannot rely on dominance due to motion` addresses this concern but only partially; it works if the FOR phi that needs to sink an instruction is handled before the FOR phi that needs to keep it in place. But the two phi's could be encountered in the other order.


I've recommitted the patch: https://github.com/llvm/llvm-project/commit/9d24933f79dd7db3e469b3c4402e076cc41418f7.

It checks for all found recurrence PHIs, if any of the previous values needs sinking and bails out in LoopVectorizationLegality. I think there's still a bit of cleanup potential, e.g. removing another check from isFirstOrderRecurrence and maybe moving it to a function (e.g. validateFirstOrderRecurrences in IVDescriptors). I think we could also handle additional cases, where the required dominance relationships still hold after sinking. Let's do that as follow ups, once the patch sticks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69228