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

Mon Sep 14 06:24:38 PDT 2020

ebevhan marked an inline comment as done.
ebevhan added inline comments.

================
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
----------------
efriedma wrote:
> ebevhan wrote:
> > efriedma wrote:
> > > 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?
> > xtn doesn't saturate, no... It just seems to truncate.
> > 
> > But... Doesn't this actually mean that it's wrong? This is supposed to saturate to i32, but it's actually converting to i64 and then truncating. That can't be right.
> > 
> > You weren't wrong about assumptions on node behavior, lowering this to FP_TO_INT clearly didn't do the right thing. So this means that vector FP_TO_INT only saturates if the scalar types are the same bit width? I wonder if the same applies to f16.
> ISD::FP_TO_SINT always has the same semantics as the IR fptosi, regardless of what the actual underlying instructions do.
Ah, certainly. I really meant FCVTZ, not FP_TO_INT.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86078