[PATCH] D104854: Introduce intrinsic llvm.isnan

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:662
+  EVT NewResultVT = TLI.getTypeToTransformTo(*DAG.getContext(), ResultVT);
+  return DAG.getNode(N->getOpcode(), DL, NewResultVT, N->getOperand(0));
+}
----------------
Don't you net to preserve the NoFPExcept flag? Same with all the other type legalization functions


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:600
+
+  SDValue Res = DAG.getNode(ISD::ISNAN, DL, ResultVT, Arg);
+  // Vectors may have a different boolean contents to scalars.  Promote the
----------------
If this is ResultVT then the Extend created next is always a NOP. Should this be MVT::i1?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:4727
+
+  if (TLI.isTypeLegal(WideResultVT)) {
+    SDValue WideNode = DAG.getNode(ISD::ISNAN, DL, WideResultVT, WideArg);
----------------
I wonder if we should be using getSetCCResultType here like WidenVecOp_SETCC?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6371
+        DAG.getNode(ISD::ISNAN, sdl, DestVT, getValue(I.getArgOperand(0)));
+    V->setFlags(Flags);
+    // If ISD::ISNAN should be expanded, do it right now, because the expansion
----------------
Why not pass flags to getNode?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6375
+    // types prior to selection.
+    if (!TLI.isOperationLegalOrCustom(ISD::ISNAN, ArgVT)) {
+      SDValue Result = TLI.expandISNAN(V.getNode(), DAG);
----------------
This breaks if we add constant folding for ISD::ISNAN to getNode.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6981
+  // V = sign bit (Sub) <=> V = (Sub < 0)
+  SDValue SignBitV = DAG.getNode(ISD::SRL, DL, IntVT, Sub,
+                                 DAG.getConstant(BitSize - 1, DL, IntVT));
----------------
Why can't we just check < 0 here? Why do we need to shift?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854