[llvm] 8e3f59b - [AArch64] Add option to enable/disable load-store renaming.

[Florian Hahn via llvm-commits]

> On 18 Feb 2020, at 09:27, Maxim Kuvyrkov <maxim.kuvyrkov at linaro.org> wrote:
> 
> On Mon, 17 Feb 2020 at 19:52, Florian Hahn <florian_hahn at apple.com <mailto:florian_hahn at apple.com>> wrote:
> Hi,
> 
> > On 17 Feb 2020, at 17:39, Maxim Kuvyrkov <maxim.kuvyrkov at linaro.org <mailto:maxim.kuvyrkov at linaro.org>> wrote:
> > 
> > Hi Florian,
> > 
> > This change regresses performance of 464.h264ref (from SPEC CPU2006) by 6% on aarch64-linux-gnu.  This happens due to 40% regression in FastFullPelBlockMotionSearch().
> > 
> > This patch looks like a no-op, but, apparently, it's not.  Is it expected that it's not a no-op?
> > 
> 
> Yes I think that is expected. This patch adds an option to disable the register renaming in the AArch64 load/store optimiser (which got added around November/December 2019) and disables it by default. This means fewer store pairs are generated probably causing the regression. Enabling aarch64-load-store-renaming should restore the performance.
> 
> The reason for disabling it is that we found a mis-compile caused by the renaming. I am still looking into tracking down the mis-compile, but unfortunately the mis-compile is in an internal app that is quite tricky to build.
> 
> For the 10.0 release, I think the option should be disabled to avoid shipping with a known issue.
> 
> For master, we could turn it on again, which might turn up a better reproducer. What do you think?
> 
> Cheers,
> Florian 
> 
> 
> I agree.  Let's leave conservative fix on release branch and re-enable on master to increase the probability of catching the bug.

SGTM. I’ll push a commit enabling it again in master a bit later. It very well be that the internal regression is unrelated/caused by UB in the source.

Cheers,
Florian

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200218/7eabeb88/attachment.html>