[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
----------------
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.)


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:8801
+  if (!isa<BuildVectorSDNode>(Op.getNode()))
+    return Op;
   if (VT.isInteger()) {
----------------
Is this change related somhow?


================
Comment at: llvm/test/CodeGen/AArch64/fptosi-sat-scalar.ll:80-90
 define i19 @test_signed_i19_f32(float %f) nounwind {
 ; CHECK-LABEL: test_signed_i19_f32:
 ; CHECK:       // %bb.0:
 ; CHECK-NEXT:    mov w8, #-931135488
 ; CHECK-NEXT:    mov w9, #65472
 ; CHECK-NEXT:    movk w9, #18559, lsl #16
 ; CHECK-NEXT:    fmov s1, w8
----------------
ebevhan wrote:
> paulwalker-arm wrote:
> > This test backs up the performance side of what I'm saying in expandFP_TO_INT_SAT.  Using the knowledge that the saturation value is exactly representable as a float is not the only thing you care about for AArch64.  You also care about "what's the most efficient way to perform the clamping operations".
> > 
> > Assuming (mainly because I haven't yet found documented proof) that a native FP_TO_INT operation on AArch64 implements the NaN->0 requirement, then I'm wondering if there's ever a good reason to perform the clamping at the input stage. Perhaps it's really a question of scalar vs vector with regards to the availability of MIN/MAX instructions.
> So perhaps we should just skip the standard expansion for these cases and only expand to the pattern you specified there. It's probably not safe to do it for all cases if we can't guarantee the saturating behavior of other FP_TO_INT operations.
> 
> > Assuming (mainly because I haven't yet found documented proof) that a native FP_TO_INT operation on AArch64 implements the NaN->0 requirement,
> 
> That is interesting, I can't find it either. I simply adapted this from the previous patchset by nikic, andI presumed that that was correct.
> Perhaps it's an undocumented behavior.
It's documented in the AArch64 reference manual, it's just sort of buried. The conversion is described in FPToFixed(), which calls FPUnpackBase().


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86078