[PATCH] D42849: [ARM] Armv8.2-A FP16 code generation (part 3/3)
Oliver Stannard via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 5 02:59:27 PST 2018
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
More information about the llvm-commits
mailing list