[PATCH] D86078: [AArch64] Improved lowering for saturating float to int.

Thu Sep 10 03:03:01 PDT 2020

ebevhan added inline comments.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:8098
   SDValue Select = FpToInt;
+  if (NativeSaturating) {
+    // If the conversion is natively saturating, avoid floating point
----------------
efriedma wrote:
> Please don't abuse the semantics of target-independent opcodes.  If the replacement operation is saturating, we should use FP_TO_SINT_SAT.  (I assume this doesn't form an infinite loop because the saturation width would be different.)
I don't think saturation width is used to determine legality, so I don't think I can emit a _SAT here, unless I've misunderstood how legalization operates.

I think I would be more inclined to simply remove NativeSaturation and do everything in Lower.

If I can't use FP_TO_INT, should there be patterns in AArch64 for selecting FP_TO_INT_SAT nodes in that case?

================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:8801
+  if (!isa<BuildVectorSDNode>(Op.getNode()))
+    return Op;
   if (VT.isInteger()) {
----------------
efriedma wrote:
> Is this change related somhow?
I got an issue here with some of the vector emissions since `getNode` with a BUILD_VECTOR can actually manage to optimize away the BUILD_VECTOR node and we end up getting something else. Then the rest of the code in this function breaks, because it assumes that Op is a BUILD_VECTOR.

Even if I change the patch after this so we don't end up with that pattern any more, I might keep this in because it's a pitfall waiting to happen.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86078