[PATCH] D15038: [ARM] Add ARMv8.2-A FP16 scalar instructions

[Oliver Stannard via llvm-commits]

olista01 added inline comments.

================
Comment at: lib/Target/ARM/Disassembler/ARMDisassembler.cpp:2213
@@ +2212,3 @@
+
+  if (U)
+    Inst.addOperand(MCOperand::createImm(ARM_AM::getAM5FP16Opc(ARM_AM::add, imm)));
----------------
jmolloy wrote:
> 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!
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.

================
Comment at: lib/Target/ARM/MCTargetDesc/ARMAddressingModes.h:679
@@ +678,3 @@
+  /// floating-point value, then return -1.
+  static inline int getFP16Imm(const APInt &Imm) {
+    uint32_t Sign = Imm.lshr(15).getZExtValue() & 1;
----------------
jmolloy wrote:
> 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.
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.

================
Comment at: lib/Target/ARM/MCTargetDesc/ARMAddressingModes.h:686
@@ +685,3 @@
+    // mantissa = (16+UInt(e:f:g:h))/16.
+    if (Mantissa & 0x3f)
+      return -1;
----------------
jmolloy wrote:
> 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; ?
"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".

================
Comment at: lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp:641
@@ +640,3 @@
+    // Offset by 4, adjusted by two due to the half-word ordering of thumb.
+    Value = Value - 4;
+    bool isAdd = true;
----------------
jmolloy wrote:
> I have no context here, but I assume that Value is definitely unsigned?
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.

================
Comment at: lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp:648
@@ +647,3 @@
+    // These values don't encode the low bit since it's always zero.
+    assert((Value & 0x1) == 0 && "invalid value for this fixup");
+    Value >>= 1;
----------------
All the other errors in this function are now reported with Ctx->reportError, I'll fix this one.

================
Comment at: lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp:1300
@@ -1294,1 +1299,3 @@
 
+/// getAddrMode5FP16OpValue - Return encoding info for 'reg +/- imm9' operand.
+uint32_t ARMMCCodeEmitter::
----------------
jmolloy wrote:
> Isn't this actually +/- imm8?
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.


Repository:
  rL LLVM

http://reviews.llvm.org/D15038