[PATCH] D38315: [ARM] Armv8.2-A FP16 code generation (part 1/2)

[Oliver Stannard via Phabricator via llvm-commits]

olista01 added inline comments.


================
Comment at: lib/Target/ARM/ARMCallingConv.td:159
 
+  CCIfType<[f16], CCBitConvertToType<i16>>,
+
----------------
Are the changes in this file needed if we're still using the clang hack for the calling convention?


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:529
+    if (Subtarget->isTargetHardFloat())
+      setOperationAction(ISD::BITCAST, MVT::i16, Custom);
+  }
----------------
If this custom lowering is correct and profitable, why not have it always enabled?


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:2485
+
+    if (Subtarget->hasFullFP16() && Subtarget->isTargetHardFloat()) {
+      // Half-precision return values can be returned like this:
----------------
I think it would be better to fix the code-generation for bitcasts generally, rather than putting in these special cases for arguments and returns. Also, this doesn't seem to be working in one of your test cases (we get store/load pairs), do you know why?


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:4949
+
+  // Half-precision arguments can be passed in like this:
+  //
----------------
Again, it would be better to fix the code-generation for bitcasts, and let the usual DAG optimisations remove the unnecessary bitcasts/truncates/extends, than to put in special cases for arguments and returns.


================
Comment at: lib/Target/ARM/MCTargetDesc/ARMBaseInfo.h:273
     // This four-bit field describes the addressing mode used.
-    AddrModeMask  = 0x1f, // The AddrMode enums are declared in ARMBaseInfo.h
+    AddrModeMask  = 0x3f, // The AddrMode enums are declared in ARMBaseInfo.h
 
----------------
Why is this needed? The new value 17 still fits in 5 bits. If this is actually needed, then do the *Shift values below need to be increased?


================
Comment at: test/CodeGen/ARM/fp16-instructions.ll:46
+
+; CHECK-SOFTFP-FULLFP16:  strh  r1, {{.*}}
+; CHECK-SOFTFP-FULLFP16:  strh  r0, {{.*}}
----------------
This looks like we're getting the default legalisation for bitcasts, so we get worse code-gen than the SOFTFP-FP16 case. Could this be fixed by adding some tablegen patterns for bitcast?


https://reviews.llvm.org/D38315