[PATCH] D25020: [ARM] Fix 26% performance regression on Cortex-A9 caused by not using VMLA/VMLS

[Evgeny Astigeevich via llvm-commits]

eastig added a comment.

In https://reviews.llvm.org/D25020#570269, @rovka wrote:

> In https://reviews.llvm.org/D25020#569200, @eastig wrote:
>
> > Hi Diana,
> >
> > Based on the results of the LNT runs I think the code checking accumulator forwarding is not needed at all.
>
>
> Which code? You're checking accumulator forwarding both in ISel and in MLxExpansion.


I mean both checks in ISel and MLxExpansion.

I traced the history of the code and discussions related to it:
http://lists.llvm.org/pipermail/llvm-dev/2013-February/059201.html
http://lists.llvm.org/pipermail/llvm-dev/2013-December/068806.html

What I've read changes the situation a little bit.
A benchmark suffered from VMLx was MILC from SPEC 2006. There were some other benchmarks but their names were not mentioned. MILC uses VFP instructions. So the problem is not SIMD specific. I'll run SPEC 2006 to check if it's still an issue.

> 
> 
>> If I am correct only SIMD VMLx instructions can have issues. So we should check only them.
>>  My thoughts:
>> 
>> - We can have features: HasSIMDVMLxHazards and HasVFPVMLxHazards. HasVMLxHazards can be built on them.
> 
> The VMLxHazards feature is enabled for Cortex-A7, A8, A9 and Swift. I think we need a better picture of the differences in behaviour between them before we rush to create more features.

Base on new facts I agree with you.

> 
> 
>> - The current checks in MLxExpansion and in ARMDAGToDAGISel::canUseVMLxForwarding are used only for SIMD instructions.
> 
> Regarding the ISel changes: those will be enabled for Cortex-A7, A8 and A9 (the intersection between HasVMLxHazards and HasVMLxForwarding). I'm a bit wary of making those changes without more benchmarking on A7 and A8 at least.

The problem is to get Cortex-A7 and Cortex-A8 hardware. We have a bare-metal Cortex-A8 board but no Cortex-A7.

So a question is: what to do if accumulator forwarding and a data hazard are detected?
The patch gives a preference for the accumulator forwarding.
>From the past discussions I see the performance problems were with VFP code. This explains why I haven't seen any changes when I added support of SIMD to the patch (not published yet). It might mean adding it's worth to add support of SIMD to the patch.

Thanks,
Evgeny


https://reviews.llvm.org/D25020