[PATCH] D130028: [SelectionDAG] Fix fptoi.sat scalable vector lowering

Mon Jul 18 14:09:35 PDT 2022

efriedma added a comment.

> I guess I had presumed a vector, as that seems to be what is used in practice. I hadn't really considered it though, just called DAG.getSetCC and DAG.getSelect with the existing operands. With getSetCCResultType from the result type, which would be vector.

Okay.

> From what I remember - A vector SELECT_CC will Expand by unrolling. There is no specific expansion code. When I added some (to turn it into a getSetcc+getSelect) the code for some tests was worse than unrolling. (There is also some MVE errors about the condition types of float vectors without ). I could add expansion code that is only for scalable vectors.

Okay.

I think I'd like to have the expansion, just so it isn't a trap for anyone deciding using SELECT_CC in the future... we can just turn it on for scalable vectors, sure.

================
Comment at: llvm/test/CodeGen/AArch64/fptosi-sat-vector.ll:1069
+; CHECK-NEXT:    mov x3, v0.d[1]
+; CHECK-NEXT:    fmov x2, d0
 ; CHECK-NEXT:    ret
----------------
dmgreen wrote:
> efriedma wrote:
> > This isn't an improvement.
> Yeah these are the i50 tests I mentioned in the summary. It doesn't help the optics that the return <4 x i50> type is passed in scalars.
> There was quite a lot of changes originally from the fallout of adding the expansion of fp_to_si_sat. I can add more to the conditions of the TLI.expandFP_TO_INT_SAT call, but then we are just trading smaller code in more common cases for some regressions in more obscure. It already felt like I was adding too many conditions. This patch was intended as a bugfix though, so perhaps it is better to keep it simpler.
> 
> Suggestions welcome.
Oh, hmm, the scalarized return is making it look worse than it actually is.

Maybe it's fine for now, then.

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

https://reviews.llvm.org/D130028