The SLP vectoriser will fail to vectorise this because it wasn't taught to emit runtime alias analysis checks. This is being addressed by Florian in https://reviews.llvm.org/D102834. We had many issues with full unrolling removing opportunities for the loop vectoriser, and the SLP missing some tricks. But with D102834<https://reviews.llvm.org/D102834> fixed, I expect this loop to be SLP vectorised and most of this pass ordering problems to disappear. Philip seems happy with this being part of GVN, and I don't have strong opinions, but GVNHoist seems like the natural place for this. An alternative strategy could thus be to integrate this into GVNHoist, enable it by default but only your simple gvn hoist algorithm (and disable the rest). That would perhaps then be a first step to address Philip's unhappiness with GVNHoist and restructure and improve it step by step. ________________________________ From: Momchil Velikov <momchil.velikov at gmail.com> Sent: 15 September 2021 12:30 To: Sjoerd Meijer <Sjoerd.Meijer at arm.com> Cc: Momchil Velikov <Momchil.Velikov at arm.com>; llvm-dev at lists.llvm.org <llvm-dev at lists.llvm.org>; Philip Reames <listmail at philipreames.com> Subject: Re: [llvm-dev] [RFC] Simple GVN hoist On Wed, Sep 15, 2021 at 12:18 PM Momchil Velikov <momchil.velikov at gmail.com> wrote:> > On Tue, Sep 14, 2021 at 7:56 PM Sjoerd Meijer via llvm-dev > <llvm-dev at lists.llvm.org> wrote: > > Fix GVNHoist. I like your patch because it's small and concise, but missing in the RFC is a discussion why we don't try to re-enable GVNHoist. I know it was briefly enabled by default, which was reverted due to correctness (or was it regressions?) problems. But if this belongs in GVNHoist, could this for example be added to GVNHoist, and only this part enabled? Not sure if that's possible as I haven't looked at GVNHoist. > > Yes, GVNHoist will do what we need[hit send too soon] So, GVNHoist will do the hoisting we want.. However, while technically possible there a few drawbacks in taking the existing GVNHoist approach: * GVNHoist is large and algorithmically complex * Even if enabled, it won't solve this specific issue - after hoisting of the loads the loop size falls below some unrolling threshold and the loop is fully unrolled - instead the loop should be loop-vectorised, IMHO - so we get a pass order issue. * After unrolling the SLP vectoriser does not vectorise the loop - so we have to fix the SLP vectoriser too. While I think all of these are worthwhile, we have on one hand some rather simple transformation (the mini-GVNHoist) vs. on the other hand the actual GVNHoist+pass manager+SLP Vectoriser, which could easily turn into a time sink with no end in sight. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210915/bee8822c/attachment.html>
On Wed, Sep 15, 2021 at 5:17 PM Sjoerd Meijer <Sjoerd.Meijer at arm.com> wrote:> The SLP vectoriser will fail to vectorise this because it wasn't taught to emit runtime alias analysis checks.It likely needs more than that - manually hoisting all the loads and adding `restrict` still does not vectorise at `-O3`: https://gcc.godbolt.org/z/z3KjaYv9d> Philip seems happy with this being part of GVN, and I don't have strong opinions, but GVNHoist seems like the natural place for this. An alternative strategy could thus be to integrate this into GVNHoist, enable it by default but only your simple gvn hoist algorithm (and disable the rest). That would perhaps then be a first step to address Philip's unhappiness with GVNHoist and restructure and improve it step by step.I'm afraid I have to disagree - GVNHoist already does all the hoisting we need, in that sense the mini-GVNhoist is not something to integrate, but a simpler alternative. If we put the issue of dealing with masked loads and stores in vectorisers aside, I see these approaches: * improve on the mini-GNVhoist as part of GVN (current proposal) * bring GVNHoist up to snuff * create a NewGVNHoist on top of NewGVN
On 9/15/21 9:17 AM, Sjoerd Meijer wrote:> > The SLP vectoriser will fail to vectorise this because it wasn't > taught to emit runtime alias analysis checks. This is being addressed > by Florian in https://reviews.llvm.org/D102834 > <https://reviews.llvm.org/D102834>. We had many issues with full > unrolling removing opportunities for the loop vectoriser, and the SLP > missing some tricks. But with D102834 > <https://reviews.llvm.org/D102834> fixed, I expect this loop to be SLP > vectorised and most of this pass ordering problems to disappear. > > Philip seems happy with this being part of GVN, and I don't have > strong opinions, but GVNHoist seems like the natural place for this. > An alternative strategy could thus be to integrate this into GVNHoist, > enable it by default but only your simple gvn hoist algorithm (and > disable the rest). That would perhaps then be a first step to address > Philip's unhappiness with GVNHoist and restructure and improve it step > by step.+1 to this point. This would be a perfectly reasonable way to implement the MemorySSA based approach in a shippable way. I'm not opposed to the non-memoryssa variant in (old) GVN, but this would be fine too. Philip> ------------------------------------------------------------------------ > *From:* Momchil Velikov <momchil.velikov at gmail.com> > *Sent:* 15 September 2021 12:30 > *To:* Sjoerd Meijer <Sjoerd.Meijer at arm.com> > *Cc:* Momchil Velikov <Momchil.Velikov at arm.com>; > llvm-dev at lists.llvm.org <llvm-dev at lists.llvm.org>; Philip Reames > <listmail at philipreames.com> > *Subject:* Re: [llvm-dev] [RFC] Simple GVN hoist > On Wed, Sep 15, 2021 at 12:18 PM Momchil Velikov > <momchil.velikov at gmail.com> wrote: > > > > On Tue, Sep 14, 2021 at 7:56 PM Sjoerd Meijer via llvm-dev > > <llvm-dev at lists.llvm.org> wrote: > > > Fix GVNHoist. I like your patch because it's small and concise, > but missing in the RFC is a discussion why we don't try to re-enable > GVNHoist. I know it was briefly enabled by default, which was reverted > due to correctness (or was it regressions?) problems. But if this > belongs in GVNHoist, could this for example be added to GVNHoist, and > only this part enabled? Not sure if that's possible as I haven't > looked at GVNHoist. > > > > Yes, GVNHoist will do what we need > > [hit send too soon] > > So, GVNHoist will do the hoisting we want.. However, while technically > possible there a few drawbacks in taking the existing GVNHoist > approach: > * GVNHoist is large and algorithmically complex > * Even if enabled, it won't solve this specific issue - after hoisting > of the loads the loop size falls below some unrolling threshold and > the loop is fully unrolled - instead the loop should be > loop-vectorised, IMHO - so we get a pass order issue. > * After unrolling the SLP vectoriser does not vectorise the loop - so > we have to fix the SLP vectoriser too. > > While I think all of these are worthwhile, we have on one hand some > rather simple transformation (the mini-GVNHoist) vs. on the other hand > the actual GVNHoist+pass manager+SLP Vectoriser, which > could easily turn into a time sink with no end in sight.-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210916/498fe92b/attachment.html>