[PATCH] D102353: [AArch64] Lower fpto*i.sat intrinsics.

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3303
+  // we can lower common saturating conversions to simple instructions.
+  assert(Op.getOpcode() == ISD::FP_TO_SINT_SAT ||
+         Op.getOpcode() == ISD::FP_TO_UINT_SAT);
----------------
Nit: I don't see precedent for checking opcodes again; I think this assert can be removed.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3314
+  assert(SatWidth <= DstWidth &&
+         "Expected saturation width smaller than result width");
+
----------------
Nit: "must be smaller or equal ...", or "cannot exceed ..."


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3317
+  if (SrcVT.isVector()) {
+    // TODO: Support lowering of NEON and SVE conversions.
+    return SDValue();
----------------
The other patch was doing this:

    if (SrcVT.isVector()) {
      SDValue Vec = LowerVectorFP_TO_INT(Op, DAG);
      if (Vec != Op)
        return Vec;
    }

Haven't looked into this, but does that make sense? Can we reuse that?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3321
+
+  if (SatWidth != DstWidth) {
+    // TODO: Saturate to SatWidth explicitly.
----------------
Nit: the coding style says that we don't need brackets around ifs if they contain one statement. So to save some space we can remove the brackets and place the TODO before the if.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3327
+  // In the absence of FP16 support, promote f32 to f16, like LowerFP_TO_INT().
+  if (SrcVT == MVT::f16 && !Subtarget->hasFullFP16()) {
+    SDLoc dl(Op);
----------------
Nit: remove the brackets, and just use `dl(Op)` directly as an argument.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3335
+  // Cases that we can emit directly.
+  if ((SrcVT == MVT::f64 || SrcVT == MVT::f32 ||
+       (SrcVT == MVT::f16 && Subtarget->hasFullFP16())) &&
----------------
Same nit, don't need the brackets.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16032
   case ISD::FP_TO_UINT:
+    // TODO: Do the same for FP_TO_*INT_SAT.
     return performFpToIntCombine(N, DAG, DCI, Subtarget);
----------------
Nit: the comment makes sense, I am not sure it belongs here though.


================
Comment at: llvm/test/CodeGen/AArch64/round-fptosi-sat-scalar.ll:43
+  %r = call half @llvm.floor.f16(half %a) nounwind readnone
+  %i = call i64 @llvm.fptosi.sat.i64.f16(half %r)
+  ret i64 %i
----------------
I haven't checked, but we don't have a f16 -> i32 variant of this?


================
Comment at: llvm/test/CodeGen/AArch64/round-fptosi-sat-scalar.ll:194
+entry:
+  %r = call half @llvm.trunc.f16(half %a) nounwind readnone
+  %i = call i32 @llvm.fptosi.sat.i32.f16(half %r)
----------------
A trunc from a f16 to a f16 should be a no-op? Do we need this? I see similar patterns below, so I must be missing something...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102353