[PATCH] D54749: Saturating float to int casts.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

Please take care of the clang-format lints.

There are some missing optimizations here.  It's probably worth teaching LegalizeVectorOps to expand the conversion into vector ops.  And for conversions that correspond to a native instruction, there's probably some more efficient code sequence.  But those can be dealt with in followups.

Making the saturation type an argument seemed a little strange at first glance, but it seems to work out reasonably well.  Maybe could change the argument to be a type, like ISD::SIGN_EXTEND_INREG.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2683
+    if (TLI.isOperationLegalOrCustom(Opcode, NewOutTy))
+      break;
+  }
----------------
This can't be the only place that uses this loop; can it be refactored?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:163
+  case ISD::FP_TO_SINT_SAT:
+    R = ScalarizeVecRes_FP_TO_XINT_SAT(N);
+    break;
----------------
Do we need a corresponding ScalarizeVecOp_FP_TO_XINT_SAT?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7987
+    SrcVT = Src.getValueType();
+  }
+
----------------
ebevhan wrote:
> I also added this to solve issues in expanding f16 FP_TO_XINT nodes to libcalls. Libcalls with a source of f16 don't seem to exist, so we can't emit such nodes if we will end up making libcalls of them.
> 
> Maybe this should check operation and type legality, though.
In theory, legalization should ensure we generate appropriate libcalls.

Maybe that doesn't actually happen at the moment, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54749