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

[James Molloy via llvm-commits]

jmolloy added a subscriber: jmolloy.
jmolloy requested changes to this revision.
jmolloy added a reviewer: jmolloy.
jmolloy added a comment.
This revision now requires changes to proceed.

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


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

================
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;
----------------
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.

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

================
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;
----------------
I have no context here, but I assume that Value is definitely unsigned?

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


Repository:
  rL LLVM

http://reviews.llvm.org/D15038