[llvm-dev] [RFC] Simple GVN hoist

[Sjoerd Meijer via llvm-dev]

>> 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

Hmmm, but it does at -O2 and not too bad from a first quick look. I don't know what that tells us though (haven't looked into details), but at O2 it does look like the SLP vectoriser is kicking in?

> I'm afraid I have to disagree - GVNHoist already does all the hoisting
> we need, in that se​nse the mini-GVNhoist is not something to
> integrate, but a simpler alternative.

The way I look at this is that we could have different modes for GVNHoist. Your simple GVN hoist could be the normal/cheap mode: simpler and more compile-time friendly, slightly less optimal results. In aggressive mode (the current stuff in GVNHoist), it could spend more compile time on more optimal results.

> 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

I am not a big fan of all these pass names like SimplePassName, NewPassName, NewerPassName, MiniPassName, etc, which is another reason why I was trying to see if we could integrate this in GVNHoist somehow (and thus for me this would be the first step to "bring GVNHoist up to snuff"). But if people are happy with this as part of GVN, I am not going to object of course.
________________________________
From: Momchil Velikov <momchil.velikov at gmail.com>
Sent: 15 September 2021 18:04
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>; florian_hahn at apple.com <florian_hahn at apple.com>
Subject: Re: [llvm-dev] [RFC] Simple GVN hoist

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210915/ca942181/attachment.html>