[PATCH] D36130: [SLP] Vectorize jumbled memory loads.

[Shahid, Asghar-ahmad via llvm-commits]

Hi Evgeny,

No problem at all:).

Just sent this patch https://reviews.llvm.org/D41324
Hope this fixes the regression issue.

Regards,
Shahid

> -----Original Message-----
> From: Evgeny Astigeevich [mailto:Evgeny.Astigeevich at arm.com]
> Sent: Saturday, December 16, 2017 8:48 PM
> To: Shahid, Asghar-ahmad <Asghar-ahmad.Shahid at amd.com>
> Cc: nd <nd at arm.com>; llvm-commits at lists.llvm.org
> Subject: Re: [PATCH] D36130: [SLP] Vectorize jumbled memory loads.
> 
> Hi Shahid,
> 
> Thank you for fixing the issue. I also want to apologise if I used a wrong
> name. I had a problem to find a correct one.
> 
> I agree with you, there is no mandatory requirement to test and to
> benchmark all targets. We usually monitor the llvm mailing lists and try to
> check performance impact of patches.
> Unfortunately the patch D36130 was not sent to llvm-commits and we did
> not  see it.
> On llvm-dev we have been asked many times to assess impact on ARM
> targets even the patch was not related to ARM at all. We always welcome
> this.
> Regarding the patch D36130, it has found several issues in different places to
> fix.
> 
> BTW, the regressed benchmarks are the part of LLVM performance testing:
> http://lnt.llvm.org/ . There are also x86 runs.
> 
> Thanks,
> Evgeny
> 
> On 16/12/2017, 12:03, "Shahid, Asghar-ahmad" <Asghar-
> ahmad.Shahid at amd.com> wrote:
> 
>     Hi Evgeny,
> 
>     My bad, I did miss to include the profitability check for extra shuffle which I
> thought is included as it was present in https://reviews.llvm.org/D26905
> 
>     I will send a patch soon to fix this.
> 
>     Regards,
>     Shahid
> 
>     > -----Original Message-----
>     > From: Shahid, Asghar-ahmad
>     > Sent: Saturday, December 16, 2017 1:23 PM
>     > To: 'Evgeny Astigeevich' <Evgeny.Astigeevich at arm.com>
>     > Cc: mgrang at codeaurora.org; diego.caballero at intel.com;
>     > hans at chromium.org; mzolotukhin at apple.com; anna at azul.com; Florian
>     > Hahn <Florian.Hahn at arm.com>; keno at juliacomputing.com;
>     > mkuper at google.com;
>     > reviews+D36130+public+cceac88dc376ed77 at reviews.llvm.org; Renato
> Golin
>     > <renato.golin at linaro.org>; Kristof Beyls <Kristof.Beyls at arm.com>;
>     > t.p.northover at gmail.com; danielcdh at gmail.com;
> zvi.rackover at intel.com;
>     > ayal.zaks at intel.com; nd <nd at arm.com>
>     > Subject: RE: [PATCH] D36130: [SLP] Vectorize jumbled memory loads.
>     >
>     > Hi Evgeny,
>     >
>     > I am not aware that it is mandatory to add tests or do the performance
>     > assessment for each and every targets. OTOH, the check for profitability
>     > happens w.r.t to the built vectorizable tree. So it seems the regressing
> tests
>     > for your target think it is profitable to vectorize.
>     >
>     > Hence, IMO, the issue seems to be either in the cost-modelling or the
>     > subsequent lowering for the given target.
>     >
>     > Regards,
>     > Shahid
>     >
>     > > -----Original Message-----
>     > > From: Evgeny Astigeevich [mailto:Evgeny.Astigeevich at arm.com]
>     > > Sent: Friday, December 15, 2017 10:56 PM
>     > > To: Shahid, Asghar-ahmad <Asghar-ahmad.Shahid at amd.com>
>     > > Cc: mgrang at codeaurora.org; diego.caballero at intel.com;
>     > > hans at chromium.org; mzolotukhin at apple.com; anna at azul.com;
> Florian
>     > Hahn
>     > > <Florian.Hahn at arm.com>; keno at juliacomputing.com;
>     > mkuper at google.com;
>     > > reviews+D36130+public+cceac88dc376ed77 at reviews.llvm.org; Renato
>     > Golin
>     > > <renato.golin at linaro.org>; Kristof Beyls <Kristof.Beyls at arm.com>;
>     > > t.p.northover at gmail.com; danielcdh at gmail.com;
> zvi.rackover at intel.com;
>     > > ayal.zaks at intel.com; nd <nd at arm.com>
>     > > Subject: Re: [PATCH] D36130: [SLP] Vectorize jumbled memory loads.
>     > >
>     > > Hi Mohammad,
>     > >
>     > > I created a problem report:
>     > > https://bugs.llvm.org/show_bug.cgi?id=35673
>     > >
>     > > I think the patch has the following issues:
>     > >
>     > > 1. No assessment of performance impact on other targets.
>     > > 2. No tests for other targets. I see it's tested only for x86.
>     > > 3. In the patch I don't see any checks if the optimization is profitable.
>     > >
>     > > What if we revert the patch to fix these issues?
>     > >
>     > > Thanks,
>     > > Evgeny Astigeevich
>     > >
>     > > On 14/12/2017, 13:32, "Evgeny Astigeevich via Phabricator"
>     > > <reviews at reviews.llvm.org> wrote:
>     > >
>     > >     eastig added a comment.
>     > >
>     > >     Hi Shahid,
>     > >
>     > >     These changes caused 27.7% and 30.2% regressions on an AArch64
>     > > Juno board (http://lnt.llvm.org/db_default/v4/nts/83681):
>     > >
>     > >     MultiSource/Benchmarks/mediabench/gsm/toast/toast: 30.20%
>     > >     MultiSource/Benchmarks/MiBench/telecomm-gsm/telecomm-gsm:
>     > 27.73%
>     > >
>     > >     We have the same benchmarks regressed on our AArch64 boards
>     > > (Cortex- A53, Cortex-A57).
>     > >
>     > >     -Evgeny Astigeevich
>     > >     The ARM Compiler Optimisation team
>     > >
>     > >
>     > >     https://reviews.llvm.org/D36130
>     > >
>     > >
>     > >
>     > >
> 
>