Paths
| Differential D23808
[X86][SSE] Add lowering to cvttpd2dq/cvttps2dq for sitofp v2f64/2f32 to 2i32 ClosedPublic Authored by RKSimon on Aug 23 2016, 10:24 AM.
Details Summary As discussed on PR28461 we currently miss the chance to lower "fptosi <2 x double> %arg to <2 x i32>" to cvttpd2dq due to its use of illegal types. This patch adds support for fptosi to 2i32 from both 2f64 and 2f32. It also recognises that cvttpd2dq zeroes the upper 64-bits of the xmm result (similar to D23797) - we still don't do this for the cvttpd2dq/cvttps2dq intrinsics - this can be done in a future patch.
Diff Detail
Event TimelineRKSimon added inline comments.
Comment Actions I propose to do the following (I wrote some code to be sure that it works) case ISD::FP_TO_SINT: {
SDValue V = LowerFP_TO_SINT(SDValue(N, 0), DAG)) {
Results.push_back(V);
return;
}SDValue X86TargetLowering::LowerFP_TO_SINT(SDValue Op, SelectionDAG &DAG) const {
MVT VT = Op.getSimpleValueType();
SDValue Src = Op.getOperand(0);
MVT SrcVT = Src.getSimpleValueType();
SDLoc DL(Op);
if (SrcVT == MVT::v2f32 && VT == MVT::v2i32) {
SDValue Idx = DAG.getIntPtrConstant(0, DL);
SDValue ExtSrc = DAG.getNode(ISD::CONCAT_VECTORS, DL, MVT::v4f32, Src,
DAG.getUNDEF(MVT::v2f32));
SDValue FpToInt = DAG.getNode(ISD::FP_TO_SINT, DL, MVT::v4i32, ExtSrc);
return DAG.getNode(ISD::EXTRACT_SUBVECTOR, DL, MVT::v2i32, FpToInt, Idx);
}
if (SrcVT == MVT::v2f64 && VT == MVT::v2i32) {
SDValue FpToInt = DAG.getNode(ISD::FP_TO_SINT, DL, MVT::v4i32, Src);
return DAG.getNode(ISD::EXTRACT_SUBVECTOR, DL, MVT::v2i32, FpToInt,
DAG.getIntPtrConstant(0, DL));
}
setOperationAction(ISD::FP_TO_SINT, MVT::v2i32, Custom);
setOperationAction(ISD::FP_TO_SINT, MVT::v2f32, Custom);And you need to add some patterns to the .td files for "fp_to_sint" instead of adding a new node type. Comment Actions
I've investigated this approach and don't agree with it. Not only would we be introducing illegal return types in lowering code (far more serious than illegal argument types) but the x87 fallback code in ReplaceNodeResults is subtly different to LowerFP_TO_SINT as it replaces the node entries in FP_TO_INTHelper.
Doing so would require us to attempt to introduce illegal types into the td file which we've avoided so far and is unlikely to work. We already do something similar from CVTPD2PS (2f64 -> 2f32) where we introduced the X86ISD::VFPROUND node. I don't consider X86ISD::CVTTPD2DQ a major intrusion. Comment Actions Actually, I wanted to put this pattern: (CVTTPD2DQrr VR128:$src)>; But if you say that it's impossible, you can use your variant. if (IsSigned) LowerFP_TO_SINT_v2i32() else LowerFP_TO_UINT_v2i32() } else // scalar types Comment Actions Elena, are you just wanting the code to be outside of the switch body in replaceNodeResults and in a separate function? I don't think it makes sense to put it into the existing LowerFP_To_SINT. That would just make that function have two different behaviors based on where its being called from.
Comment Actions
That is the role of these Lower* functions to provide custom solution for legal and and illegal types. We do the same in LowerSINT_TO_FP(). But if you think that it does not make sense, please feel free to proceed.
This revision is now accepted and ready to land.Oct 17 2016, 8:46 PM Closed by commit rL284459: [X86][SSE] Add lowering to cvttpd2dq/cvttps2dq for sitofp v2f64/2f32 to 2i32 (authored by RKSimon). · Explain WhyOct 18 2016, 12:51 AM This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 74799 lib/Target/X86/X86ISelLowering.h
lib/Target/X86/X86ISelLowering.cpp
lib/Target/X86/X86InstrAVX512.td
lib/Target/X86/X86InstrFragmentsSIMD.td
lib/Target/X86/X86InstrSSE.td
lib/Target/X86/X86TargetTransformInfo.cpp
test/Analysis/CostModel/X86/fptosi.ll
test/CodeGen/X86/vec_fp_to_int.ll
test/Transforms/SLPVectorizer/X86/fptosi.ll
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Why you do not call Lower_FP_TO_SINT ?