[PATCH] D69275: Add constrained int->FP intrinsics

[Kevin P. Neal via Phabricator via llvm-commits]

kpn marked 6 inline comments as done.
kpn added a comment.

Huh. I forgot to submit my comments earlier.

Anyway, I've merged in many changes from D71130 <https://reviews.llvm.org/D71130>, I've made a few changes as requested in LegalizeDAG.cpp, and I added the missing call to ExpandUINT_TO_FLOAT(). Now I'm seeing X86::VMULPDYrr die when running the vector-constrained-fp-intrinsics.ll test on the v4i32 type. It seems that in InstrEmitter.cpp it dies at the assertion at line 838 because NumOperands is 3. That's as far as I've gotten today.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:178
   SDValue PromoteLegalINT_TO_FP(SDValue LegalOp, EVT DestVT, bool isSigned,
                                 const SDLoc &dl);
   void PromoteLegalFP_TO_INT(SDNode *N, const SDLoc &dl,
----------------
uweigand wrote:
> craig.topper wrote:
> > uweigand wrote:
> > > kpn wrote:
> > > > uweigand wrote:
> > > > > Hmm, wouldn't we also need to update this routine?  Or can we say that promotion is not appropriate for handling strict semantics anyway?
> > > > I don't have a test for it so I didn't change it.
> > > > 
> > > > Can we guarantee the result would be rounded back down? Seems like promotion would be invalid without that guarantee.
> > > Right.  In general promotion is not appropriate for strict semantics because you don't get the right exceptions (for overflow etc.).
> > Promotion is definitely bad for fp->int, but is there really an issue for int->fp? We're just going to use a bigger int for the input. If the small int was going to overflow, it should still overflow when its extended.
> Ah, you're right.  This should be fine.
> 
> @kpn: If you're looking for a test case, this would most likely involve something like a i16->f32 conversion.
Agreed. I'm working on LegalizeDAG.cpp right now. I notice X86 already has Promote for i8 and i16.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2397
+    SDValue Sub;
+    if (Node->isStrictFPOpcode()) {
+      Sub = DAG.getNode(ISD::STRICT_FSUB, dl, {MVT::f64, MVT::Other},
----------------
craig.topper wrote:
> Can we merge the 2 strict fp ifs here? The only thing we do between them is declare a new variable.
Done.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2405
+    if (Node->isStrictFPOpcode()) {
+      if (!DestVT.bitsEq(Sub.getValueType())) {
+        std::pair<SDValue, SDValue> ResultPair;
----------------
craig.topper wrote:
> Why is this not just DestVT != Sub.getValueType()
Eh, I figured it'd be more clear this way. I'll change it.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:1160
 SDValue VectorLegalizer::ExpandUINT_TO_FLOAT(SDValue Op) {
-  EVT VT = Op.getOperand(0).getValueType();
+  bool IsStrict = Op.getNode()->isStrictFPOpcode();
+  unsigned OpNo = IsStrict ? 1 : 0;
----------------
craig.topper wrote:
> Can a strict node get here? The call site in vectorLegalizer::Expand only checks for the non-strict node.
Ah, I must have lost the change to call here, possibly with D69887 going in.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:18704
   if (Subtarget.hasSSE3() && shouldUseHorizontalOp(true, DAG, Subtarget)) {
+    // FIXME: Do we need a STRICT version of FHADD?
     Result = DAG.getNode(X86ISD::FHADD, dl, MVT::v2f64, Sub, Sub);
----------------
craig.topper wrote:
> We probably do need a strict version of FHADD, but until we have that we should just go to the shuffle + STRICT_FADD code below rather than silently dropping the chain.
Change lifted from D71130.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:18989
                                   OffsetSlot, MachinePointerInfo());
     SDValue Fild = BuildFILD(Op, MVT::i64, Store2, StackSlot, DAG);
     return Fild;
----------------
craig.topper wrote:
> This looks out of date. I recently changed this to return a std::pair of Result and Chain so it was obvious that the chain result was intended as an output. So we need a merge values here for strict fp now. This should be reflected in https://reviews.llvm.org/D71130
Yup, I just needed to rebase.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69275/new/

https://reviews.llvm.org/D69275