This is an archive of the discontinued LLVM Phabricator instance.

[ARM][MVE][Intrinsics] Add VMINAQ, VMINNMAQ, VMAXAQ, VMAXNMAQ intrinsics.
ClosedPublic

Authored by MarkMurrayARM on Jan 15 2020, 5:21 AM.

Details

Summary

Add VMINAQ, VMINNMAQ, VMAXAQ, VMAXNMAQ intrinsics and unit tests.

Event Timeline

MarkMurrayARM created this revision.Jan 15 2020, 5:21 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 15 2020, 5:21 AM
simon_tatham added inline comments.Jan 15 2020, 6:31 AM
clang/include/clang/Basic/arm_mve.td
289

I wonder if we should implement at least the simple case (integer and unpredicated) using standard IR nodes instead of an IR intrinsic?

We already implement vmaxq using an icmp and a select. We haven't implemented vabsq yet, but when we do, it will surely be done in a similar way, to take advantage of the existing pattern matching showcased in llvm/test/CodeGen/Thumb2/mve-abs.ll. So possibly we should code-generate vmaxaq(a,b) as if it was vmaxq(a, vabsq(b)), and write a more complicated isel pattern that will match that whole tree?

The advantage would be that if a user had literally written a combination of vmaxq and vabsq, codegen would be able to fold them together into a single instruction at compile time.

The FP versions might make sense to do the same way, using the standard @llvm.fabs IR intrinsic for the abs part.

Much better impementation after review feedback.

MarkMurrayARM marked an inline comment as done.Jan 15 2020, 8:18 AM
simon_tatham accepted this revision.Jan 15 2020, 8:39 AM

LGTM this time. Thanks for the rewrite!

This revision is now accepted and ready to land.Jan 15 2020, 8:39 AM
This revision was automatically updated to reflect the committed changes.

Nice one. Good to see codegen changes coming out of these intrinsics.

It took a while for me to figure out what the integer instruction was doing. That's a strange one.

The fp case I have a question about below.

llvm/lib/Target/ARM/ARMInstrMVE.td
3658

If I'm reading the ARMARM correctly, the fp case seems to preform the abs on both operands.

MarkMurrayARM marked an inline comment as done.Jan 16 2020, 1:50 AM
MarkMurrayARM added inline comments.
llvm/lib/Target/ARM/ARMInstrMVE.td
3658

My bad. Fix coming under separate cover.