[PATCH] D42954: [ARM] f16 conversions

[Oliver Stannard via Phabricator via llvm-commits]

olista01 added inline comments.


================
Comment at: lib/Target/ARM/ARMInstrVFP.td:682
 
 def : FullFP16Pat<(f32 (fpextend HPR:$Sm)),
                   (VCVTBHS (COPY_TO_REGCLASS HPR:$Sm, SPR))>;
----------------
The instruction only requires HasFP16, so why does the pattern need FullFP16? (same question multiple times in this file)


================
Comment at: lib/Target/ARM/ARMInstrVFP.td:694
+def : FullFP16Pat<(f16 (fpround SPR:$Sm)),
+                  (COPY_TO_REGCLASS (VCVTBSH SPR:$Sm), SPR)>;
+def : FP16Pat<(fp_to_f16 SPR:$a),
----------------
Should this COPY_TO_REGCLASS target HPR, since the output is an f16?


================
Comment at: lib/Target/ARM/ARMInstrVFP.td:725
+def : FullFP16Pat<(f64 (fpextend HPR:$Sm)),
+                  (VCVTBHD (COPY_TO_REGCLASS HPR:$Sm, DPR))>;
+def : FP16Pat<(f64 (f16_to_fp GPR:$a)),
----------------
VCVTBHD takes the input in an SPR, so should the COPY_TO_REGCLASS target SPR?


================
Comment at: test/CodeGen/ARM/fp16-instructions.ll:237
+
+define i32 @h2d_d2h(i32 %h.coerce, double %d) {
+entry:
----------------
It would be better to have each test in its own function, rather then combining them like this.


https://reviews.llvm.org/D42954