[llvm-dev] [RFC] Simple GVN hoist

[Philip Reames via llvm-dev]

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>