[LegalizeTypes][LegalDAG][X86] Copy SDNodeFlags or at least fpexcept flag when legalizing strict fp nodes

[craig.topper]

Summary

This is needed to preserve the FP except information. These were the obvious easy cases. More patches definitely needed.

We should consider changing our representation and the default semantics for this property. Using what is effectively a fast math flag is dangerous since the API was designed for them to be lost without hurting correctness.

Random thoughts on how we might do that
-Change the polarity and require all the existing non-strict transforms to pass noexcept everywhere. This seems like it would require touching a lot of places and carefully auditing.
-Change the polarity, but make it only have meaning on STRICT nodes. Ignore it on other nodes.
-Change the polarity, but ignore it inside functions that don't have the strictfp attribute.

[craig.topper]

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

7434

Not adding flags here, because none of the other getNode functions do for this same spot.

[craig.topper]

Adding some more cases where flags are being lost. I'll retitle this review later. And I still need to add tests.

[pengfei]

How about the status of handing down the flags? Do we have a solution for flags in all situation?

[craig.topper]

In D71590#2326856, @pengfei wrote:

How about the status of handing down the flags? Do we have a solution for flags in all situation?

https://reviews.llvm.org/D87361 made things easier, but we still need to do work at every location. Every transform has to be looked at for fast math safety.

llvm/include/llvm/CodeGen/SelectionDAG.h

#endif
 
   /// Gets or creates the specified node.
   ///
   SDValue getNode(unsigned Opcode, const SDLoc &DL, EVT VT,
                   ArrayRef<SDUse> Ops);
   SDValue getNode(unsigned Opcode, const SDLoc &DL, EVT VT,
                   ArrayRef<SDValue> Ops, const SDNodeFlags Flags = SDNodeFlags());
   SDValue getNode(unsigned Opcode, const SDLoc &DL, ArrayRef<EVT> ResultTys,
-                  ArrayRef<SDValue> Ops);
+                  ArrayRef<SDValue> Ops, const SDNodeFlags Flags = SDNodeFlags());
   SDValue getNode(unsigned Opcode, const SDLoc &DL, SDVTList VTList,
-                  ArrayRef<SDValue> Ops);
+                  ArrayRef<SDValue> Ops, const SDNodeFlags Flags = SDNodeFlags());
 
   // Specialize based on number of operands.
   SDValue getNode(unsigned Opcode, const SDLoc &DL, EVT VT);
   SDValue getNode(unsigned Opcode, const SDLoc &DL, EVT VT, SDValue Operand,
                   const SDNodeFlags Flags = SDNodeFlags());
   SDValue getNode(unsigned Opcode, const SDLoc &DL, EVT VT, SDValue N1,
                   SDValue N2, const SDNodeFlags Flags = SDNodeFlags());
   SDValue getNode(unsigned Opcode, const SDLoc &DL, EVT VT, SDValue N1,

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

while (true) {
     }
 
     // Otherwise, try a larger type.
   }
 
   // Okay, we found the operation and type to use.  Zero extend our input to the
   // desired type then run the operation on it.
   if (IsStrict) {
+    SDNodeFlags Flags;
+    Flags.setFPExcept(N->getFlags().hasFPExcept());
     SDValue Res =
         DAG.getNode(OpToUse, dl, {DestVT, MVT::Other},
                     {N->getOperand(0),
                      DAG.getNode(IsSigned ? ISD::SIGN_EXTEND : ISD::ZERO_EXTEND,
-                                 dl, NewInTy, LegalOp)});
+                                 dl, NewInTy, LegalOp)}, Flags);
     Results.push_back(Res);
     Results.push_back(Res.getValue(1));
   }
 
   Results.push_back(
       DAG.getNode(OpToUse, dl, DestVT,
                   DAG.getNode(IsSigned ? ISD::SIGN_EXTEND : ISD::ZERO_EXTEND,
                               dl, NewInTy, LegalOp)));
if (!IsSigned && TLI.isOperationLegalOrCustom(OpToUse, NewOutTy))
       break;
 
     // Otherwise, try a larger type.
   }
 
   // Okay, we found the operation and type to use.
   SDValue Operation;
   if (IsStrict) {
-    SDVTList VTs = DAG.getVTList(NewOutTy, MVT::Other);
-    Operation = DAG.getNode(OpToUse, dl, VTs, N->getOperand(0), LegalOp);
+    SDNodeFlags Flags;
+    Flags.setFPExcept(N->getFlags().hasFPExcept());
+    Operation = DAG.getNode(OpToUse, dl, { NewOutTy, MVT::Other },
+                            { N->getOperand(0), LegalOp }, Flags);
   } else
     Operation = DAG.getNode(OpToUse, dl, NewOutTy, LegalOp);
 
   // Truncate the result of the extended FP_TO_*INT operation to the desired
   // size.
   SDValue Trunc = DAG.getNode(ISD::TRUNCATE, dl, DestVT, Operation);
   Results.push_back(Trunc);
   if (IsStrict)

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

for (unsigned i = 1; i < NumOpers; ++i) {
     SDValue Oper = N->getOperand(i);
 
     if (Oper.getValueType().isVector())
       Oper = GetScalarizedVector(Oper);
 
     Opers[i] = Oper;
   }
 
-  SDValue Result = DAG.getNode(N->getOpcode(), dl, ValueVTs, Opers);
+  SDValue Result = DAG.getNode(N->getOpcode(), dl, ValueVTs, Opers,
+                               N->getFlags());
 
   // Legalize the chain result - switch anything that used the old chain to
   // use the new one.
   ReplaceValueWith(SDValue(N, 1), Result.getValue(1));
   return Result;
 }
 
 SDValue DAGTypeLegalizer::ScalarizeVecRes_OverflowOp(SDNode *N,
if (InVT.isVector()) {
       else
         std::tie(OpLo, OpHi) = DAG.SplitVectorOperand(N, i);
     }
 
     OpsLo[i] = OpLo;
     OpsHi[i] = OpHi;
   }
 
+  const SDNodeFlags Flags = N->getFlags();
   EVT LoValueVTs[] = {LoVT, MVT::Other};
   EVT HiValueVTs[] = {HiVT, MVT::Other};
-  Lo = DAG.getNode(N->getOpcode(), dl, LoValueVTs, OpsLo);
-  Hi = DAG.getNode(N->getOpcode(), dl, HiValueVTs, OpsHi);
+  Lo = DAG.getNode(N->getOpcode(), dl, LoValueVTs, OpsLo, Flags);
+  Hi = DAG.getNode(N->getOpcode(), dl, HiValueVTs, OpsHi, Flags);
 
   // Build a factor node to remember that this Op is independent of the
   // other one.
   Chain = DAG.getNode(ISD::TokenFactor, dl, MVT::Other,
                       Lo.getValue(1), Hi.getValue(1));
 
   // Legalize the chain result - switch anything that used the old chain to
   // use the new one.
for (unsigned j = 1, e = N->getNumOperands(); j != e; ++j) {
         Operands[j] =
             DAG.getNode(ISD::EXTRACT_VECTOR_ELT, dl, OperandEltVT, Operand,
                     DAG.getConstant(i, dl, TLI.getVectorIdxTy(
                           DAG.getDataLayout())));
       } else {
         Operands[j] = Operand;
       }
     }
-    SDValue Scalar = DAG.getNode(N->getOpcode(), dl, ChainVTs, Operands);
-    Scalar.getNode()->setFlags(N->getFlags());
+    SDValue Scalar = DAG.getNode(N->getOpcode(), dl, ChainVTs, Operands,
+                                 N->getFlags());
 
     //Add in the scalar as well as its chain value to the
     //result vectors.
     Scalars.push_back(Scalar);
     Chains.push_back(Scalar.getValue(1));
   }
 
   for (; i < ResNE; ++i)

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

SDValue SelectionDAG::getNode(unsigned Opcode, const SDLoc &DL, EVT VT,
 
   InsertNode(N);
   SDValue V(N, 0);
   NewSDValueDbgMsg(V, "Creating new node: ", this);
   return V;
 }
 
 SDValue SelectionDAG::getNode(unsigned Opcode, const SDLoc &DL,
-                              ArrayRef<EVT> ResultTys, ArrayRef<SDValue> Ops) {
-  return getNode(Opcode, DL, getVTList(ResultTys), Ops);
+                              ArrayRef<EVT> ResultTys, ArrayRef<SDValue> Ops,
+                              const SDNodeFlags Flags) {
+  return getNode(Opcode, DL, getVTList(ResultTys), Ops, Flags);
 }
 
 SDValue SelectionDAG::getNode(unsigned Opcode, const SDLoc &DL, SDVTList VTList,
-                              ArrayRef<SDValue> Ops) {
+                              ArrayRef<SDValue> Ops, const SDNodeFlags Flags) {
   if (VTList.NumVTs == 1)
-    return getNode(Opcode, DL, VTList.VTs[0], Ops);
+    return getNode(Opcode, DL, VTList.VTs[0], Ops, Flags);
 
   switch (Opcode) {
   case ISD::STRICT_FP_EXTEND:
     assert(VTList.NumVTs == 2 && Ops.size() == 2 &&
            "Invalid STRICT_FP_EXTEND!");
     assert(VTList.VTs[0].isFloatingPoint() &&
            Ops[1].getValueType().isFloatingPoint() && "Invalid FP cast!");
     assert(VTList.VTs[0].isVector() == Ops[1].getValueType().isVector() &&
#endif
   }
 
   // Memoize the node unless it returns a flag.
   SDNode *N;
   if (VTList.VTs[VTList.NumVTs-1] != MVT::Glue) {
     FoldingSetNodeID ID;
     AddNodeIDNode(ID, Opcode, VTList, Ops);
     void *IP = nullptr;
-    if (SDNode *E = FindNodeOrInsertPos(ID, DL, IP))
+    if (SDNode *E = FindNodeOrInsertPos(ID, DL, IP)) {
+      E->intersectFlagsWith(Flags);
       return SDValue(E, 0);
+    }
 
     N = newSDNode<SDNode>(Opcode, DL.getIROrder(), DL.getDebugLoc(), VTList);
+    N->setFlags(Flags);
     createOperands(N, Ops);
     CSEMap.InsertNode(N, IP);
   } else {
     N = newSDNode<SDNode>(Opcode, DL.getIROrder(), DL.getDebugLoc(), VTList);
     createOperands(N, Ops);
   }
   InsertNode(N);
   SDValue V(N, 0);
   NewSDValueDbgMsg(V, "Creating new node: ", this);
   return V;
 }
 

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

bool TargetLowering::expandFP_TO_UINT(SDNode *Node, SDValue &Result,
   // the destination signmask can't be represented by the float, so we can
   // just use FP_TO_SINT directly.
   const fltSemantics &APFSem = DAG.EVTToAPFloatSemantics(SrcVT);
   APFloat APF(APFSem, APInt::getNullValue(SrcVT.getScalarSizeInBits()));
   APInt SignMask = APInt::getSignMask(DstVT.getScalarSizeInBits());
   if (APFloat::opOverflow &
       APF.convertFromAPInt(SignMask, false, APFloat::rmNearestTiesToEven)) {
     if (Node->isStrictFPOpcode()) {
+      SDNodeFlags Flags;
+      Flags.setFPExcept(Node->getFlags().hasFPExcept());
       Result = DAG.getNode(ISD::STRICT_FP_TO_SINT, dl, { DstVT, MVT::Other }, 
-                           { Node->getOperand(0), Src }); 
+                           { Node->getOperand(0), Src }, Flags);
       Chain = Result.getValue(1);
     } else
       Result = DAG.getNode(ISD::FP_TO_SINT, dl, DstVT, Src);
     return true;
   }
 
   SDValue Cst = DAG.getConstantFP(APF, dl, SrcVT);
+  // FIXME: Does this need to be a strict comparision?
   SDValue Sel = DAG.getSetCC(dl, SetCCVT, Src, Cst, ISD::SETLT);
 
   bool Strict = Node->isStrictFPOpcode() ||
                 shouldUseStrictFP_TO_INT(SrcVT, DstVT, /*IsSigned*/ false);
 
   if (Strict) {
     // Expand based on maximum range of FP_TO_SINT, if the value exceeds the
     // signmask then offset (the result of which should be fully representable).
     // Sel = Src < 0x8000000000000000
     // FltOfs = select Sel, 0, 0x8000000000000000
     // IntOfs = select Sel, 0, 0x8000000000000000
     // Result = fp_to_sint(Src - FltOfs) ^ IntOfs
 
     // TODO: Should any fast-math-flags be set for the FSUB?
     SDValue FltOfs = DAG.getSelect(dl, SrcVT, Sel,
                                    DAG.getConstantFP(0.0, dl, SrcVT), Cst);
     SDValue IntOfs = DAG.getSelect(dl, DstVT, Sel,
                                    DAG.getConstant(0, dl, DstVT),
                                    DAG.getConstant(SignMask, dl, DstVT));
     SDValue SInt;
     if (Node->isStrictFPOpcode()) {
+      SDNodeFlags Flags;
+      Flags.setFPExcept(Node->getFlags().hasFPExcept());
       SDValue Val = DAG.getNode(ISD::STRICT_FSUB, dl, { SrcVT, MVT::Other }, 
-                                { Node->getOperand(0), Src, FltOfs });
+                                { Node->getOperand(0), Src, FltOfs }, Flags);
       SInt = DAG.getNode(ISD::STRICT_FP_TO_SINT, dl, { DstVT, MVT::Other }, 
-                         { Val.getValue(1), Val });
+                         { Val.getValue(1), Val }, Flags);
       Chain = SInt.getValue(1);
     } else {
       SDValue Val = DAG.getNode(ISD::FSUB, dl, SrcVT, Src, FltOfs);
       SInt = DAG.getNode(ISD::FP_TO_SINT, dl, DstVT, Val);
     }
     Result = DAG.getNode(ISD::XOR, dl, DstVT, SInt, IntOfs);
   } else {
     // Expand based on maximum range of FP_TO_SINT:

llvm/lib/Target/X86/X86ISelLowering.cpp

if (SrcVT == MVT::i64 && UseSSEReg && Subtarget.is64Bit())
     return Op;
 
   if (SDValue V = LowerI64IntToFP_AVX512DQ(Op, DAG, Subtarget))
     return V;
 
   // SSE doesn't have an i16 conversion so we need to promote.
   if (SrcVT == MVT::i16 && (UseSSEReg || VT == MVT::f128)) {
     SDValue Ext = DAG.getNode(ISD::SIGN_EXTEND, dl, MVT::i32, Src);
-    if (IsStrict)
+    if (IsStrict) {
+      SDNodeFlags Flags;
+      Flags.setFPExcept(Op->getFlags().hasFPExcept());
       return DAG.getNode(ISD::STRICT_SINT_TO_FP, dl, {VT, MVT::Other},
-                         {Op.getOperand(0), Ext});
+                         {Op.getOperand(0), Ext}, Flags);
+    }
 
     return DAG.getNode(ISD::SINT_TO_FP, dl, VT, Ext);
   }
 
   if (VT == MVT::f128)
     return LowerF128Call(Op, DAG, RTLIB::getSINTTOFP(SrcVT, VT));
 
   SDValue ValueToStore = Src;
else if (TheVT == MVT::f80)
       Status = Thresh.convert(APFloat::x87DoubleExtended(),
                               APFloat::rmNearestTiesToEven, &LosesInfo);
 
     assert(Status == APFloat::opOK && !LosesInfo &&
            "FP conversion should have been exact");
 
     SDValue ThreshVal = DAG.getConstantFP(Thresh, DL, TheVT);
 
+    // FIXME: Does this need to be a strict compare?
     SDValue Cmp = DAG.getSetCC(DL,
                                getSetCCResultType(DAG.getDataLayout(),
                                                   *DAG.getContext(), TheVT),
                                Value, ThreshVal, ISD::SETLT);
     Adjust = DAG.getSelect(DL, MVT::i64, Cmp,
                            DAG.getConstant(0, DL, MVT::i64),
                            DAG.getConstant(APInt::getSignMask(64),
                                            DL, MVT::i64));
     SDValue FltOfs = DAG.getSelect(DL, TheVT, Cmp,
                                    DAG.getConstantFP(0.0, DL, TheVT),
                                    ThreshVal);
 
     if (IsStrict) {
+      SDNodeFlags Flags;
+      Flags.setFPExcept(Op->getFlags().hasFPExcept());
       Value = DAG.getNode(ISD::STRICT_FSUB, DL, { TheVT, MVT::Other},
-                          { Chain, Value, FltOfs });
+                          { Chain, Value, FltOfs }, Flags);
       Chain = Value.getValue(1);
     } else
       Value = DAG.getNode(ISD::FSUB, DL, TheVT, Value, FltOfs);
   }
 
   MachinePointerInfo MPI = MachinePointerInfo::getFixedStack(MF, SSFI);
 
   // FIXME This causes a redundant load/store if the SSE-class value is already
if (isScalarFPTypeInSSEReg(TheVT)) {
     Value = DAG.getMemIntrinsicNode(X86ISD::FLD, DL, Tys, Ops, TheVT, MMO);
     Chain = Value.getValue(1);
   }
 
   // Build the FP_TO_INT*_IN_MEM
   MachineMemOperand *MMO = MF.getMachineMemOperand(
       MPI, MachineMemOperand::MOStore, MemSize, MemSize);
   SDValue Ops[] = { Chain, Value, StackSlot };
+  // FIXME: How do we get the fpexcept flag onto this node?
   SDValue FIST = DAG.getMemIntrinsicNode(X86ISD::FP_TO_INT_IN_MEM, DL,
                                          DAG.getVTList(MVT::Other),
                                          Ops, DstTy, MMO);
 
   SDValue Res = DAG.getLoad(Op.getValueType(), SDLoc(Op), FIST, StackSlot, MPI);
   Chain = Res.getValue(1);
 
   // If we need an unsigned fixup, XOR the result with adjust.
if (!IsSigned && UseSSEReg) {
     assert(VT == MVT::i32 && "Unexpected VT!");
 
     // Promote i32 to i64 and use a signed operation on 64-bit targets.
     // FIXME: This does not generate an invalid exception if the input does not
     // fit in i32. PR44019
     if (Subtarget.is64Bit()) {
       SDValue Res, Chain;
       if (IsStrict) {
+        SDNodeFlags Flags;
+        Flags.setFPExcept(Op->getFlags().hasFPExcept());
         Res = DAG.getNode(ISD::STRICT_FP_TO_SINT, dl, { MVT::i64, MVT::Other},
-                          { Op.getOperand(0), Src });
+                          { Op.getOperand(0), Src }, Flags);
         Chain = Res.getValue(1);
       } else
         Res = DAG.getNode(ISD::FP_TO_SINT, dl, MVT::i64, Src);
 
       Res = DAG.getNode(ISD::TRUNCATE, dl, VT, Res);
       if (IsStrict)
         return DAG.getMergeValues({ Res, Chain }, dl);
       return Res;
     }
 
     // Use default expansion for SSE1/2 targets without SSE3. With SSE3 we can
     // use fisttp which will be handled later.
     if (!Subtarget.hasSSE3())
       return SDValue();
   }
 
   // Promote i16 to i32 if we can use a SSE operation or the type is f128.
   // FIXME: This does not generate an invalid exception if the input does not
   // fit in i16. PR44019
   if (VT == MVT::i16 && (UseSSEReg || SrcVT == MVT::f128)) {
     assert(IsSigned && "Expected i16 FP_TO_UINT to have been promoted!");
     SDValue Res, Chain;
     if (IsStrict) {
+      SDNodeFlags Flags;
+      Flags.setFPExcept(Op->getFlags().hasFPExcept());
       Res = DAG.getNode(ISD::STRICT_FP_TO_SINT, dl, { MVT::i32, MVT::Other},
-                        { Op.getOperand(0), Src });
+                        { Op.getOperand(0), Src }, Flags);
       Chain = Res.getValue(1);
     } else
       Res = DAG.getNode(ISD::FP_TO_SINT, dl, MVT::i32, Src);
 
     Res = DAG.getNode(ISD::TRUNCATE, dl, VT, Res);
     if (IsStrict)
       return DAG.getMergeValues({ Res, Chain }, dl);
     return Res;