[PATCH] Add support to promote f16 to f32

[Ahmed Bougacha]

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:1591
@@ +1590,3 @@
+    if (RetVT == MVT::f32 ||
+        RetVT == MVT::f64) {
+
----------------
I don't think you need this, just always pick some opcode, and let the operation legalizer decide it doesn't like a type combination.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:1596
@@ +1595,3 @@
+  }
+  else if (RetVT == MVT::f16) {
+    if (OpVT == MVT::f32 ||
----------------
'else' with '{'.  Same elsewhere

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:1604
@@ +1603,3 @@
+
+  assert (false && "Attempt at an invalid promotion-related conversion");
+  return ISD::DELETED_NODE;
----------------
llvm_unreachable, or better still, report_fatal_error, as this is actually reachable from purpose-crafted IR?  Same below.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:1608
@@ +1607,3 @@
+
+static RTLIB::Libcall GetPromotionLibcall(EVT OpVT, EVT RetVT) {
+  if (OpVT == MVT::f16) {
----------------
As I said on the ML, not sure if it'll work, but you should just let the ops legalizer pick a libcall for the FP_TO_FP nodes.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:1626
@@ +1625,3 @@
+
+static SDValue GetPromotedValue(SelectionDAG &DAG, const TargetLowering &TLI,
+                                const SDValue &Operand, EVT OpVT, EVT RetVT,
----------------
I didn't notice this, and found the name a bit confusing, thinking this was some magic legalizer method.  Might be good to have a more descriptive name, making it explicit we actually promote here, rather than get a previously promoted value as GetPromotedInt/Float does.

The fact that it goes both ways is also a bit confusing, but I get that avoids duplication.  For such a short function, it might be worth doing two separate versions, perhaps?

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:1700
@@ +1699,3 @@
+  // If node's operand is itself an extend/promotion operation, directly promote
+  // to VT.  For e.g., convert f16 -> f32 -> f64 to f16 -> f64.
+  if (Op.getNode()->getOpcode() == ISD::FP16_TO_FP) {
----------------
I'm not sure what to think of this.  Could we catch this in a DAGCombine instead?

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:1701
@@ +1700,3 @@
+  // to VT.  For e.g., convert f16 -> f32 -> f64 to f16 -> f64.
+  if (Op.getNode()->getOpcode() == ISD::FP16_TO_FP) {
+    Op = Op.getNode()->getOperand(0);
----------------
'Op->getOpcode()' instead, as you already do for getValueType. Same elsewhere

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:1704
@@ +1703,3 @@
+    return GetPromotedValue(DAG, TLI, Op, MVT::f16, MVT::f32, SDLoc(N));
+    // return DAG.getNode(ISD::FP16_TO_FP, SDLoc(N), VT, Op);
+  }
----------------
Dead

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:1753
@@ +1752,3 @@
+  if (Src->getOpcode() == ISD::FP16_TO_FP) {
+    // Optimization: collapse FP16->FP32 followed by FP32 -> FP16.
+    NewVal = Src->getOperand(0);
----------------
Should these optimizations be DAGCombines instead?

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:1804
@@ +1803,3 @@
+
+    /// FROUND
+
----------------
What about it? :P

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:1809
@@ +1808,3 @@
+    case ISD::FDIV:
+    case ISD::FMA:
+    case ISD::FMAXNUM:
----------------
This isn't a binop.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:1982
@@ +1981,3 @@
+  // Round promoted float to desired precision
+  SDValue Round = GetPromotedValue(DAG, TLI, Op, NVT, VT, dl);
+  // Promote it back to the legal output type
----------------
I don't follow.  This is about rounding to integers, and has nothing to do with FP, right?  Why not simply GetPromotedFloat?  Or, for that matter, just reusing PromoteFloatRes_UnaryOp.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:260
@@ +259,3 @@
+      return DAG.getNode(ISD::AssertZext, dl,
+                         NOutVT, Trunc, DAG.getValueType(OutVT));
+    }
----------------
The line break seems odd, should there be more in the first line?

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:262
@@ -254,1 +261,3 @@
+    }
+  }
   case TargetLowering::TypeExpandInteger:
----------------
Whether the fallthrough is intended or not, I think an explicit 'break' or comment would be useful.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeTypes.h:512
@@ +511,3 @@
+
+  SDValue GetPromotedFloat(SDValue Op) {
+    SDValue &PromotedOp = PromotedFloats[Op];
----------------
Move this to the .cpp?

http://reviews.llvm.org/D8755

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