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

[Jacob Bramley via Phabricator via llvm-commits]

jbramley added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3317
+  if (SrcVT.isVector()) {
+    // TODO: Support lowering of NEON and SVE conversions.
+    return SDValue();
----------------
SjoerdMeijer wrote:
> 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?
The other patch also extends `LowerVectorFP_TO_INT` to handle the saturating semantics. It's probably a reasonable approach, and didn't see any objections there.


================
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
----------------
SjoerdMeijer wrote:
> I haven't checked, but we don't have a f16 -> i32 variant of this?
Isn't that `@llvm.fptosi.sat.i32.f16`, in `@testmswh`, above?


================
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)
----------------
SjoerdMeijer wrote:
> 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...
We need it because `trunc` [truncates to an integer](https://llvm.org/docs/LangRef.html#llvm-trunc-intrinsic), leaving the result in the same FP format (like `frintz` in the fallback machine instruction sequence). It's not a no-op.

All of the tests in this file are round + convert sequences, like `round.conv.ll`. I added other tests to `fptosi-sat-scalar.ll` (etc) for the simple, standalone conversions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102353