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

[Oliver Stannard via Phabricator via llvm-commits]

olista01 accepted this revision.
olista01 added a comment.
This revision is now accepted and ready to land.

LGTM with a few nits (see inline comments), but please give 24 hours for other time zones to comment before committing.



================
Comment at: lib/Target/ARM/ARMInstrVFP.td:531
+                  [(arm_cmpfp HPR:$Sd, HPR:$Sm, (i32 1))]>,
+                  Requires<[HasFullFP16]>;
 
----------------
The AHuI class already has the HasFullFP16 predicate, so is this needed?

(same question many more times in this file)


================
Comment at: lib/Target/ARM/ARMInstrVFP.td:784
     def UH : AHuInp<0b11101, 0b11, 0b1100, 0b01, 0,
-                    (outs SPR:$Sd), (ins SPR:$Sm),
+                    (outs HPR:$Sd), (ins HPR:$Sm),
                     NoItinerary, !strconcat("vcvt", opc, ".u32.f16\t$Sd, $Sm"),
----------------
Output should be SPR?


================
Comment at: lib/Target/ARM/ARMInstrVFP.td:1383
+def : VFPNoNEONPat<(f16 (uint_to_fp GPR:$a)),
+                   (VUITOH (COPY_TO_REGCLASS GPR:$a, HPR))>;
+
----------------
Should this copy to the SPR regclass, since the input is 32-bit?


================
Comment at: test/CodeGen/ARM/fp16-instructions.ll:99
+entry:
+  %0 = bitcast float %F.coerce to i32
+  %tmp.0.extract.trunc = trunc i32 %0 to i16
----------------
It might be better to write these tests as loading/storing the fp16 values, rather than doing all of this bit-casting, to simplify the tests. However, if you're going to do native support for fp16 args/returns in the backend soon, I'd be fine with committing these as-is for now knowing that they'll be simplified even further by that.


https://reviews.llvm.org/D42849