This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Add ARMv8.2-A FP16 vector instructions
ClosedPublic

Authored by olista01 on Nov 26 2015, 5:10 AM.

Details

Reviewers
t.p.northover
ab
Summary

ARMv8.2-A adds 16-bit floating point versions of all existing SIMD
floating-point instructions. This is an optional extension, so all of
these instructions require the FeatureFullFP16 subtarget feature.

Note that VFP without SIMD is not a valid combination for any version of
ARMv8-A, but I have ensured that these instructions all depend on both
FeatureNEON and FeatureFullFP16 for consistency.

The ".2h" vector type specifier is now legal (for the scalar pairwise
reduction instructions), so some unrelated tests have been modified as
different error messages are emitted. This is not a problem as the
invalid operands are still caught.

Diff Detail

Event Timeline

olista01 updated this revision to Diff 41236.Nov 26 2015, 5:10 AM
olista01 retitled this revision from to [AArch64] Add ARMv8.2-A FP16 vector instructions.
olista01 updated this object.
olista01 added a reviewer: t.p.northover.
olista01 set the repository for this revision to rL LLVM.
olista01 added a subscriber: llvm-commits.
ab added a subscriber: ab.Nov 27 2015, 4:00 AM

In general, no objections.

What do you think of introducing more base classes for the H and non-H variants? For instance, for BaseSIMDThreeSameVector, you could have something like BaseSIMDThreeSameVectorH which just overrides (or passes up, if we also added a BaseSIMDThreeSameVectorSD) the correct bit. That could help avoid a lot of the noise, and keep the descriptions a little cleaner, I think.

Another thing is, many of the patterns are in standalone multiclasses, and would need f16 variants as well. I'll submit a patch if you don't beat me to it!

Also, you might want to split out more of the scalar instructions.

lib/Target/AArch64/AArch64InstrFormats.td
4592

80-col? for Tied as well

Another really minor nit: what about ordering them like so: "size, size2, opcode"?

4981โ€“4984

reflow?

5700

SIMDThreeScalarSD -> SIMDThreeScalarHSD?
Or, as we do elsewhere -better IMO-, SIMDFPThreeScalar?

5883

\t before operands?

6053

SIMDAcrossLanesS -> HS ?

6943โ€“6945

alignment?

olista01 marked 4 inline comments as done.Nov 27 2015, 6:16 AM

What do you think of introducing more base classes for the H and non-H variants? For instance, for BaseSIMDThreeSameVector, you could have something like BaseSIMDThreeSameVectorH which just overrides (or passes up, if we also added a BaseSIMDThreeSameVectorSD) the correct bit. That could help avoid a lot of the noise, and keep the descriptions a little cleaner, I think.

I'm not sure about this. It would make this patch smaller, but would complicate the inheritance trees by having an extra layer of base classes for the FP instructions (which have different parameters), which I don't think would be clearer in the end.

Another thing is, many of the patterns are in standalone multiclasses, and would need f16 variants as well. I'll submit a patch if you don't beat me to it!

I don't have any further patches for this (I just put patterns in where copying the existing ones was obviously correct), so go ahead.

lib/Target/AArch64/AArch64InstrFormats.td
5700

I also prefer the latter, since *HSD is still wrong for targets without FullFP16.

6053

Changed to SIMDFPAcrossLanes

olista01 updated this revision to Diff 41300.Nov 27 2015, 6:17 AM
olista01 removed rL LLVM as the repository for this revision.
olista01 marked an inline comment as done.
ab accepted this revision.Dec 4 2015, 10:08 AM
ab added a reviewer: ab.

LGTM

This revision is now accepted and ready to land.Dec 4 2015, 10:08 AM
olista01 closed this revision.Dec 8 2015, 4:19 AM

Committed revision 255010.