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

[Eli Friedman via Phabricator via llvm-commits]

efriedma 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
----------------
ebevhan wrote:
> 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?
If you've marked the legality of a node "Custom", the target determines whether it's legal.  If the custom lowering hook just returns the input node, that will tell the legalizer that the node is legal.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3050
+        DAG.getNode(IsSigned ? ISD::FP_TO_SINT : ISD::FP_TO_UINT, dl, DstVT,
+                    Clamped);
+    return FpToInt;
----------------
ebevhan wrote:
> Do you think it's still wrong to use FP_TO_INT in these lowerings?
My concern is that you're making assumptions that doesn't match the specified semantics of FP_TO_SINT.  This is confusing, and some code could see this operation and perform some illegal transform on it.  (This sort of usage has caused miscompiles in the past.)

int_aarch64_neon_fcvtzs has the right semantics, or you could introduce a new target-specific node, or you could generate another FP_TO_SINT_SAT.  I guess I'd slightly prefer a new node.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:8801
+  if (!isa<BuildVectorSDNode>(Op.getNode()))
+    return Op;
   if (VT.isInteger()) {
----------------
ebevhan wrote:
> 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.
Oh, hmm, strange that nobody else has hit it.  I guess it's fine.


================
Comment at: llvm/test/CodeGen/AArch64/fptosi-sat-vector.ll:175
+; CHECK-NEXT:    fcvtzs v0.2d, v0.2d
+; CHECK-NEXT:    xtn v0.2s, v0.2d
 ; CHECK-NEXT:    ret
----------------
ebevhan wrote:
> efriedma wrote:
> > This should probably just be "fcvtzs v0.2d, v0.2d; sqxtn v0.2s, v0.2d".
> Is sqxtn ever selected without intrinsics? I would assume that a simple minmax and trunc pattern would catch it, but that doesn't happen.
Not sure off the top of my head, but I wouldn't be surprised if the answer is no.  Nobody has spent much time on saturation patterns.

The current patch generates xtn?  That isn't saturating, I think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86078