[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

In D104854#2841505 <https://reviews.llvm.org/D104854#2841505>, @efriedma wrote:

> I'd like to see some test coverage for all the floating-point types (half, bfloat16, ppc_fp128).

Tests for half are added to aarch64-fpclass.ll, new file was created to test ppc_fp128. As for bfloat16, tests are added to aarch64-fpclass.ll but only in strictfp mode. Without this attribute codegen creates unordered compare operation, which is not supported for bfloat16.



================
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));
+}
----------------
craig.topper wrote:
> Don't you net to preserve the NoFPExcept flag? Same with all the other type legalization functions
Yes, it is more correct way. Updated 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
----------------
craig.topper wrote:
> If this is ResultVT then the Extend created next is always a NOP. Should this be MVT::i1?
Indeed. Thank you!


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


================
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
----------------
craig.topper wrote:
> Why not pass flags to getNode?
Yes, it should be set via 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);
----------------
craig.topper wrote:
> This breaks if we add constant folding for ISD::ISNAN to getNode.
Now expansion occurs before getNode.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6967
+  // NaN has all exp bits set and a non zero significand. Therefore:
+  // isnan(V) == ((exp mask - (abs(V) & exp mask)) < 0)
+  unsigned BitSize = OperandVT.getScalarSizeInBits();
----------------
thopre wrote:
> I seem to have made a mistake when I wrote this.
Thank you! I updated the comment.


================
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));
----------------
craig.topper wrote:
> Why can't we just check < 0 here? Why do we need to shift?
It seems the shift is not needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854