<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On 18 Feb 2020, at 09:27, Maxim Kuvyrkov <<a href="mailto:maxim.kuvyrkov@linaro.org" class="">maxim.kuvyrkov@linaro.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><div dir="ltr" class="">On Mon, 17 Feb 2020 at 19:52, Florian Hahn <<a href="mailto:florian_hahn@apple.com" target="_blank" class="">florian_hahn@apple.com</a>> wrote:<br class=""></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br class="">
<br class="">
> On 17 Feb 2020, at 17:39, Maxim Kuvyrkov <<a href="mailto:maxim.kuvyrkov@linaro.org" target="_blank" class="">maxim.kuvyrkov@linaro.org</a>> wrote:<br class="">
> <br class="">
> Hi Florian,<br class="">
> <br class="">
> This change regresses performance of 464.h264ref (from SPEC CPU2006) by 6% on aarch64-linux-gnu. This happens due to 40% regression in FastFullPelBlockMotionSearch().<br class="">
> <br class="">
> This patch looks like a no-op, but, apparently, it's not. Is it expected that it's not a no-op?<br class="">
> <br class="">
<br class="">
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.<br class="">
<br class="">
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.<br class="">
<br class="">
For the 10.0 release, I think the option should be disabled to avoid shipping with a known issue.<br class="">
<br class="">
For master, we could turn it on again, which might turn up a better reproducer. What do you think?<br class="">
<br class="">
Cheers,<br class="">
Florian <br class="">
<br class="">
</blockquote></div><div class=""><br class=""></div><div class="">I agree. Let's leave conservative fix on release branch and re-enable on master to increase the probability of catching the bug.</div></div></div></blockquote><br class=""></div><div>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.</div><div><br class=""></div><div>Cheers,</div><div>Florian</div><br class=""></body></html>