This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Add ARMv8.2-A FP16 scalar instructions
ClosedPublic

Authored by olista01 on Nov 27 2015, 4:49 AM.

Details

Summary

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

The assembly for these instructions uses S registers (AArch32 does not
have H registers), but the instructions have ".f16" type specifiers
rather than ".f32" or ".f64". The top 16 bits of each source register
are ignored, and the top 16 bits of the destination register are set to
zero.

These instructions are mostly the same as the 32- and 64-bit versions,
but they use coprocessor 9 rather than 10 and 11.

Two new instructions, VMOVX and VINS, have been added to allow packing
and extracting two 16-bit floats stored in the top and bottom halves of
an S register.

New fixup kinds have been added for the PC-relative load and store
instructions, but no ELF relocations have been added as they have a
range of 512 bytes.

Diff Detail

Event Timeline

olista01 updated this revision to Diff 41290.Nov 27 2015, 4:49 AM
olista01 retitled this revision from to [ARM] Add ARMv8.2-A FP16 scalar instructions.
olista01 updated this object.
olista01 added reviewers: t.p.northover, ab.
olista01 set the repository for this revision to rL LLVM.
olista01 added a subscriber: llvm-commits.
jmolloy requested changes to this revision.Dec 15 2015, 2:51 AM
jmolloy added a reviewer: jmolloy.
jmolloy added a subscriber: jmolloy.

Hi Oliver,

This is difficult to review as we don't have the spec to cross-reference with. However most of the tablegen looks fairly mechanical and I'm happy with that.

I have comments about some of the encoding/decoding though.

Cheers,

James

lib/Target/ARM/Disassembler/ARMDisassembler.cpp
2213

I was going to say that this seems really strange, but then I read the block comment in the diff below (this is an operation concatenated with an always-positive integer).

Can you please add a comment here describing the format a little, just so it doesn't seem like you've treated a field as a ones-complement integer by mistake? I think naming the variable "U" makes it even more likely to think you've made a mistake, because it sounds like it should be representing signedness!

lib/Target/ARM/MCTargetDesc/ARMAddressingModes.h
679

Is this deliberately sign extending? It seems to be in the return statement but I can't quite work out why it would need to.

686

This looks dodgy. So if the mantissa is "1" (one bit of mantissa) we'd return -1 here.

Shouldn't this be if (Mantissa & ~0xfUL) return -1; ?

lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
641

I have no context here, but I assume that Value is definitely unsigned?

lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp
1300

Isn't this actually +/- imm8?

This revision now requires changes to proceed.Dec 15 2015, 2:51 AM
olista01 added inline comments.Dec 15 2015, 3:46 AM
lib/Target/ARM/Disassembler/ARMDisassembler.cpp
2213

I think the comment above the implementation of getAM5FP16Opc covers the encoding format (though I'll expand it to make it clear that "operation" is add or subtract), I don't think that needs duplicating here. The variable name "U" comes from the ARMARM (the same name is used in the 32- and 64-bit VLDR/VSTR instruction, in the published v8-A ARMARM), but I agree that the code is not very clear on it's own, so I'll add a comment.

lib/Target/ARM/MCTargetDesc/ARMAddressingModes.h
679

This won't ever actually get sign-extended, as only the bottom 8 bits can be set in the return statement. This is the same function signature as getFP32Imm and getFP64Imm, which both also return 8 bit values.

686

"Mantissa" currently holds 10 bits, a sit was extracted from an FP16 value, bit we can only encode the top 4 bits of that. If any of the bottom 6 bits are set, we return -1 to represent "could not encode".

lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
641

Value here is of type uint64_t, but actually contains a signed value (this is the case for all fixups in this switch). We interpret it as int64_t a few lines down to check if it is negative, and set isAdd appropriately.

648

All the other errors in this function are now reported with Ctx->reportError, I'll fix this one.

lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp
1300

The two comments for getAddrMode5OpValue in this file don't agree with each other (imm8 vs imm10), I'll update them all to be consistent.

olista01 updated this revision to Diff 42829.Dec 15 2015, 3:49 AM
olista01 edited edge metadata.
olista01 removed rL LLVM as the repository for this revision.
jmolloy accepted this revision.Dec 16 2015, 3:13 AM
jmolloy edited edge metadata.

With your changes, LGTM.

lib/Target/ARM/MCTargetDesc/ARMAddressingModes.h
686

Ah, the *top* 4 bits. I see, OK.

This revision is now accepted and ready to land.Dec 16 2015, 3:13 AM
This revision was automatically updated to reflect the committed changes.