[MIPS] OperationAction for FP_TO_FP16, FP16_TO_FP

[pirama]

Summary

Set operation action for FP16 conversion opcodes, so the Op legalizer
can choose the gnu_* libcalls for Mips.

[pirama]

[MIPS] OperationAction for FP_TO_FP16, FP16_TO_FP

Set operation action for FP16 conversion opcodes, so the Op legalizer
can choose the gnu_* libcalls for Mips.

[pirama]

This is related to http://reviews.llvm.org/D8755, which adds support to consider FP16 types as load/store-only types.

[ab]

Do you need this for f64 as well? Also, please add some tests, to make sure we actually get to the libcalls.

[dsanders]

Thanks for the patch and sorry for the delay in looking at it (we've just had two public holidays in the UK). The change looks right to me for f32 but as Ahmed says it needs a test case checking that it emits the call to the library.

[pirama]

Ok. I'll do that after I land http://reviews.llvm.org/D8755 since I'd need that to write tests for this patch.

[pirama]

• Added loadext and truncstore actions for f16
• Added tests to ensure that libcalls get inserted

[dsanders]

LGTM with a small nit in the test.

lib/Target/Mips/MipsISelLowering.cpp

226–229 ↗

(On Diff #23943)

While iterating, this will do 3 strange calls:

setLoadExtAction(ISD::EXTLOAD, MVT::f16, MVT::f32, Expand);
setLoadExtAction(ISD::EXTLOAD, MVT::f16, MVT::f16, Expand);
setLoadExtAction(ISD::EXTLOAD, MVT::f32, MVT::f32, Expand);

I assume it isn't a problem to do this since EXTLOAD doesn't make sense when the memory type is larger (or the same as) the value type. It might be better to list the valid cases like we do for setTruncStoreAction() below. I don't mind which approach is taken.

test/CodeGen/Mips/fp16-promote.ll

3–5 ↗

(On Diff #23943)

Nit: These two lines aren't necessary and it's best not to duplicate the datalayout in the test in case we ever change it. The bit I think might change at some point is the vector part since our vectors only require the elements to be aligned and not the whole vector.

64–71 ↗

(On Diff #23943)

Just a comment, no change required: This might be a little fragile (the exact order of the output may vary) but I don't see a good way to make it more robust at the moment.

[pirama]

Remove datalayout from the tests, and use CHECK-DAG for fpext_double test.

lib/Target/Mips/MipsISelLowering.cpp

226–229 ↗

(On Diff #23943)

fp_valuetypes has larger FP types (f80, f128 etc). Enumerating all of them will be tedious and skipping these types is a divergence from current behavior.

The only strange call introduced by this patch is the second one you listed. Such an EXTLOAD should never be generated, so I'll leave this as is.

test/CodeGen/Mips/fp16-promote.ll

3–5 ↗

(On Diff #23943)

Done

64–71 ↗

(On Diff #23943)

I'll switch to CHECK-DAG for this test. It's still not robust enough, but is better than the current state.

[dsanders]

In future, please don't commit after a conditional LGTM if the conditions haven't been met. If you disagree with the conditions, you should discuss them further.

lib/Target/Mips/MipsISelLowering.cpp

226–229 ↗

(On Diff #23943)

fp_valuetypes has larger FP types (f80, f128 etc). Enumerating all of them will be tedious and skipping these types is a divergence from current behavior.

Good point. Iterating makes sense in that case.

The only strange call introduced by this patch is the second one you listed. Such an EXTLOAD should never be generated, so I'll leave this as is.

The first one is odd because it 'extends' from a f32 to a f16 which is actually a truncation operation.
The third one is odd for the same reason as the second one. It 'extends' to the same type as in memory.

I do agree that such EXTLOAD's shouldn't be generated though.

test/CodeGen/Mips/fp16-promote.ll

64–71 ↗

(On Diff #23943)

I didn't suggest CHECK-DAG here because it doesn't do what you'd expect when the match strings are the same. All the 'CHECK-LIBCALL-DAG: cvt.d.s' will match at the same position in the file (the first instance of 'cvt.d.s'). This is because the search for CHECK-DAG matches are all matched (starting from the same position in the file) before ordering violations are determined.

For example, the following file will pass because both CHECK-DAG's match the same 'foo'.

; CHECK-DAG: fo{{o}}
; CHECK-DAG: fo{{o}}
foo

[pirama]

In D8804#158957, @dsanders wrote:

In future, please don't commit after a conditional LGTM if the conditions haven't been met. If you disagree with the conditions, you should discuss them further.

My apologies Daniel. I assumed an "Accepted" status in Phabricator carries over across minor updates, and that my last patch was minor. I was wrong on both counts. In the future, I'll wait for a LGTM on all patches.

I'll address the CHECK-DAG concern in a different patch and link it back to here.

test/CodeGen/Mips/fp16-promote.ll

64–71 ↗

(On Diff #23943)

You are right. In fact, I found out about this in a different context, but missed it here. I think the right thing to do here is to change CHECK-DAG to CHECK and add a comment noting the fragility of this test.

[dsanders]

No worries. It turned out that I agreed with most of your replies anyway :-).

I'll address the CHECK-DAG concern in a different patch and link it back to
here.

Sounds good to me. Thanks.

llvm/trunk/lib/Target/Mips/MipsISelLowering.cpp

MipsTargetLowering::MipsTargetLowering(const MipsTargetMachine &TM,
 
   // Load extented operations for i1 types must be promoted
   for (MVT VT : MVT::integer_valuetypes()) {
     setLoadExtAction(ISD::EXTLOAD,  VT, MVT::i1,  Promote);
     setLoadExtAction(ISD::ZEXTLOAD, VT, MVT::i1,  Promote);
     setLoadExtAction(ISD::SEXTLOAD, VT, MVT::i1,  Promote);
   }
 
-  // MIPS doesn't have extending float->double load/store
-  for (MVT VT : MVT::fp_valuetypes())
+  // MIPS doesn't have extending float->double load/store.  Set LoadExtAction
+  // for f32, f16
+  for (MVT VT : MVT::fp_valuetypes()) {
     setLoadExtAction(ISD::EXTLOAD, VT, MVT::f32, Expand);
+    setLoadExtAction(ISD::EXTLOAD, VT, MVT::f16, Expand);
+  }
+
+  // Set LoadExtAction for f16 vectors to Expand
+  for (MVT VT : MVT::fp_vector_valuetypes()) {
+    MVT F16VT = MVT::getVectorVT(MVT::f16, VT.getVectorNumElements());
+    if (F16VT.isValid())
+      setLoadExtAction(ISD::EXTLOAD, VT, F16VT, Expand);
+  }
+
+  setTruncStoreAction(MVT::f32, MVT::f16, Expand);
+  setTruncStoreAction(MVT::f64, MVT::f16, Expand);
+
   setTruncStoreAction(MVT::f64, MVT::f32, Expand);
 
   // Used by legalize types to correctly generate the setcc result.
   // Without this, every float setcc comes with a AND/OR with the result,
   // we don't want this, since the fpcmp result goes to a flag register,
   // which is used implicitly by brcond and select operations.
   AddPromotedToType(ISD::SETCC, MVT::i1, MVT::i32);
 
MipsTargetLowering::MipsTargetLowering(const MipsTargetMachine &TM,
   setOperationAction(ISD::FLOG2,             MVT::f32,   Expand);
   setOperationAction(ISD::FLOG10,            MVT::f32,   Expand);
   setOperationAction(ISD::FEXP,              MVT::f32,   Expand);
   setOperationAction(ISD::FMA,               MVT::f32,   Expand);
   setOperationAction(ISD::FMA,               MVT::f64,   Expand);
   setOperationAction(ISD::FREM,              MVT::f32,   Expand);
   setOperationAction(ISD::FREM,              MVT::f64,   Expand);
 
+  // Lower f16 conversion operations into library calls
+  setOperationAction(ISD::FP16_TO_FP,        MVT::f32,   Expand);
+  setOperationAction(ISD::FP_TO_FP16,        MVT::f32,   Expand);
+  setOperationAction(ISD::FP16_TO_FP,        MVT::f64,   Expand);
+  setOperationAction(ISD::FP_TO_FP16,        MVT::f64,   Expand);
+
   setOperationAction(ISD::EH_RETURN, MVT::Other, Custom);
 
   setOperationAction(ISD::VASTART,           MVT::Other, Custom);
   setOperationAction(ISD::VAARG,             MVT::Other, Custom);
   setOperationAction(ISD::VACOPY,            MVT::Other, Expand);
   setOperationAction(ISD::VAEND,             MVT::Other, Expand);
 
   // Use the default for now

llvm/trunk/test/CodeGen/Mips/fp16-promote.ll

+; RUN: llc -asm-verbose=false -mtriple=mipsel-linux-gnueabi < %s | FileCheck %s -check-prefix=CHECK-LIBCALL
+
+; CHECK-LIBCALL-LABEL: test_fadd:
+; CHECK-LIBCALL: %call16(__gnu_h2f_ieee)
+; CHECK-LIBCALL: %call16(__gnu_h2f_ieee)
+; CHECK-LIBCALL-DAG: add.s
+; CHECK-LIBCALL-DAG: %call16(__gnu_f2h_ieee)
+define void @test_fadd(half* %p, half* %q) #0 {
+  %a = load half, half* %p, align 2
+  %b = load half, half* %q, align 2
+  %r = fadd half %a, %b
+  store half %r, half* %p
+  ret void
+}
+
+; CHECK-LIBCALL-LABEL: test_fpext_float:
+; CHECK-LIBCALL: %call16(__gnu_h2f_ieee)
+define float @test_fpext_float(half* %p) {
+  %a = load half, half* %p, align 2
+  %r = fpext half %a to float
+  ret float %r
+}
+
+; CHECK-LIBCALL-LABEL: test_fpext_double:
+; CHECK-LIBCALL: %call16(__gnu_h2f_ieee)
+; CHECK-LIBCALL: cvt.d.s
+define double @test_fpext_double(half* %p) {
+  %a = load half, half* %p, align 2
+  %r = fpext half %a to double
+  ret double %r
+}
+
+; CHECK-LIBCALL-LABEL: test_fptrunc_float:
+; CHECK-LIBCALL: %call16(__gnu_f2h_ieee)
+define void @test_fptrunc_float(float %f, half* %p) #0 {
+  %a = fptrunc float %f to half
+  store half %a, half* %p
+  ret void
+}
+
+; CHECK-LIBCALL-LABEL: test_fptrunc_double:
+; CHECK-LIBCALL: %call16(__truncdfhf2)
+define void @test_fptrunc_double(double %d, half* %p) #0 {
+  %a = fptrunc double %d to half
+  store half %a, half* %p
+  ret void
+}
+
+; CHECK-LIBCALL-LABEL: test_vec_fpext_float:
+; CHECK-LIBCALL: %call16(__gnu_h2f_ieee)
+; CHECK-LIBCALL: %call16(__gnu_h2f_ieee)
+; CHECK-LIBCALL: %call16(__gnu_h2f_ieee)
+; CHECK-LIBCALL: %call16(__gnu_h2f_ieee)
+define <4 x float> @test_vec_fpext_float(<4 x half>* %p) #0 {
+  %a = load <4 x half>, <4 x half>* %p, align 8
+  %b = fpext <4 x half> %a to <4 x float>
+  ret <4 x float> %b
+}
+
+; CHECK-LIBCALL-LABEL: test_vec_fpext_double:
+; CHECK-LIBCALL-DAG: %call16(__gnu_h2f_ieee)
+; CHECK-LIBCALL-DAG: %call16(__gnu_h2f_ieee)
+; CHECK-LIBCALL-DAG: %call16(__gnu_h2f_ieee)
+; CHECK-LIBCALL-DAG: %call16(__gnu_h2f_ieee)
+; CHECK-LIBCALL-DAG: cvt.d.s
+; CHECK-LIBCALL-DAG: cvt.d.s
+; CHECK-LIBCALL-DAG: cvt.d.s
+; CHECK-LIBCALL: cvt.d.s
+define <4 x double> @test_vec_fpext_double(<4 x half>* %p) #0 {
+  %a = load <4 x half>, <4 x half>* %p, align 8
+  %b = fpext <4 x half> %a to <4 x double>
+  ret <4 x double> %b
+}
+
+; CHECK-LIBCALL-LABEL: test_vec_fptrunc_float:
+; CHECK-LIBCALL: %call16(__gnu_f2h_ieee)
+; CHECK-LIBCALL: %call16(__gnu_f2h_ieee)
+; CHECK-LIBCALL: %call16(__gnu_f2h_ieee)
+; CHECK-LIBCALL: %call16(__gnu_f2h_ieee)
+define void @test_vec_fptrunc_float(<4 x float> %a, <4 x half>* %p) #0 {
+  %b = fptrunc <4 x float> %a to <4 x half>
+  store <4 x half> %b, <4 x half>* %p, align 8
+  ret void
+}
+
+; CHECK-LIBCALL-LABEL: test_vec_fptrunc_double:
+; CHECK-LIBCALL: %call16(__truncdfhf2)
+; CHECK-LIBCALL: %call16(__truncdfhf2)
+; CHECK-LIBCALL: %call16(__truncdfhf2)
+; CHECK-LIBCALL: %call16(__truncdfhf2)
+define void @test_vec_fptrunc_double(<4 x double> %a, <4 x half>* %p) #0 {
+  %b = fptrunc <4 x double> %a to <4 x half>
+  store <4 x half> %b, <4 x half>* %p, align 8
+  ret void
+}
+