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

[Evgeny Astigeevich via llvm-commits]

eastig added inline comments.


> rovka wrote in ARMISelDAGToDAG.cpp:423
> I don't understand the first part of the comment, it's either incomplete or could use some rephrasing.

The comment the function is out of date and does not reflect the latest changes.

> rovka wrote in ARMISelDAGToDAG.cpp:462
> Why return false for vectors? AFAICT the Cortex-A9 manual says accumulator forwarding should apply for VQDMLA/VQDMLS. I see this assumption everywhere in the patch - maybe there should be a comment somewhere explaining it?

Yes, it should work according to the documentation. I'll add support of these cases and try to check if forwarding works.

> rovka wrote in MLxExpansionPass.cpp:69
> Did you run clang-format on this?

No.
Thank you for reminding about this. I always forget about clang-format.

> rovka wrote in MLxExpansionPass.cpp:235
> This looks a bit awkward and it doesn't seem to be on a very cold path either - maybe add a private flag and set it in runOnMachineFunction?

I agree with you it does not look good. I usually add flags if values are needed more than once.
I'll add a flag.

> rovka wrote in fml.ll:2
> Could you add some run lines for cores that don't have accumulator forwarding, so we can test the other behavior as well? Also, can you rename the file to something more representative?

I'll do.

> rovka wrote in fml.ll:4
> You should add a CHECK-LABEL directive for each function to make sure you're matching the expected instructions and not others appearing further down (doesn't seem likely now, but people may append tests to this file in the future).

I'll do.

> rovka wrote in fml.ll:19
> Shouldn't this snippet have an expectation too? (Ditto in the other functions)

The purpose of the snippet is to use defined values.  I'll make it simpler.

https://reviews.llvm.org/D25020