[PATCH] [MIPS] OperationAction for FP_TO_FP16, FP16_TO_FP

[Daniel Sanders]

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.


REPOSITORY
  rL LLVM

================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:226-229
@@ -226,1 +225,6 @@
+  // for f32, f16
+  for (MVT VT : MVT::fp_valuetypes()) {
     setLoadExtAction(ISD::EXTLOAD, VT, MVT::f32, Expand);
+    setLoadExtAction(ISD::EXTLOAD, VT, MVT::f16, Expand);
+  }
+
----------------
pirama wrote:
> dsanders wrote:
> > 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.
> 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.
> 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.

================
Comment at: test/CodeGen/Mips/fp16-promote.ll:64-71
@@ +63,10 @@
+; CHECK-LIBCALL-LABEL: test_vec_fpext_double:
+; CHECK-LIBCALL: %call16(__gnu_h2f_ieee)
+; CHECK-LIBCALL: %call16(__gnu_h2f_ieee)
+; CHECK-LIBCALL: %call16(__gnu_h2f_ieee)
+; CHECK-LIBCALL: cvt.d.s
+; CHECK-LIBCALL: cvt.d.s
+; CHECK-LIBCALL: cvt.d.s
+; CHECK-LIBCALL: %call16(__gnu_h2f_ieee)
+; CHECK-LIBCALL: cvt.d.s
+define <4 x double> @test_vec_fpext_double(<4 x half>* %p) #0 {
----------------
pirama wrote:
> dsanders wrote:
> > 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.
> I'll switch to CHECK-DAG for this test.  It's still not robust enough, but is better than the current state. 
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

http://reviews.llvm.org/D8804

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/