[PATCH] [X86] Updates to X86 backend for f16 promotion

Ahmed Bougacha ahmed.bougacha at gmail.com
Thu May 7 15:28:17 PDT 2015


Looking at this again, the tests should be stricter, I think.  Operands, perhaps -NEXT (on X86, you shouldn't have the scheduling/allocation problems you had last time; ARM is .. special), at the very least for the scalar tests.

With that, LGTM, thanks!


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:17367-17373
@@ -17356,2 +17366,9 @@
   }
+  case ISD::FP_EXTEND: {
+    // Right now, only MVT::v2f32 has OperationAction for FP_EXTEND.
+    // No other ValueType for FP_EXTEND should reach this point.
+    assert(N->getValueType(0) == MVT::v2f32 &&
+           "Do not know how to legalize this Node");
+    return;
+  }
   case ISD::INTRINSIC_W_CHAIN: {
----------------
pirama wrote:
> ab wrote:
> > pirama wrote:
> > > ab wrote:
> > > > Does v2f32 even hit this?  Without this patch it should trigger the default unreachable, why would that change?
> > > Around line 924 in this file:
> > > 
> > > ```
> > > setOperationAction(ISD::FP_EXTEND,          MVT::v2f32, Custom);
> > > ```
> > > is what necessitates this.  v2f32 will be a valuetype of an FP_EXTEND node only when the source type is smaller (i.e. v2f16).  I am not sure why the OperationAction is set as above, but it triggers the unreachable error for FP_EXTEND from v2f16.  The included test for vec4 extend fails because vec4 is split during legalization.  Now that I think about it, I should add a vec2 test so this test doesn't rot if vec4 is legal in the future.
> > How about removing that line, and removing the FP_EXTEND block as well?  Both seem pretty pointless IMO.
> Removing the operation action causes test/CodeGen/X86/pr11334.ll to fail.  For this test, DAGTypeLegalizer::WidenVectorOperand calls CustomLowerNode based on the operand's value type which eventually causes the v2f32 to be expanded to a v4f32 and use a VFPEXT (in LowerFP_EXTEND).
> 
> I understood that operation action is always defined for the value type of a node and not on its operand's types.  But, looks like that's not the case.
Ah, that's nasty. (yes, the operation action is keyed off of different types depending on who's asking..)

Your change is fine then, I don't have any better idea right now. 

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:17517
@@ -17508,1 +17516,3 @@
+      break;
+    // fallthrough in the false-branch
   case ISD::FP_TO_UINT: {
----------------
Just "fallthrough" ?

================
Comment at: test/CodeGen/X86/half.ll:119
@@ +118,3 @@
+; CHECK-FP16: movabsq
+; CHECK-FP16: xorg
+; CHECK-FP16: vcvttss2si
----------------
'g' -> 'q'

http://reviews.llvm.org/D9092

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






More information about the llvm-commits mailing list