[PATCH] D54749: Saturating float to int casts: Basics [1/n]

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Thank you for picking this up. Some pointers from my side:

- I would recommend against including GlobalISel support in the initial implementation. Unless trivial, SDAG and GlobalISel changes should never be made in the same patch, as the reviewers for these parts of the codebase are essentially disjoint.
- You may want to replace the VT specifying the saturation width with a simple constant integer operand. This is the approach that the fixed-point ISD opcodes went with, and it should make legalization a bit simpler.
- I originally restricted this patch to a minimum viable implementation, in the hope that it would make review easier and get this landed quickly. Given how things turned out, it might make sense to put the soft float and vector legalization (which are part of D54696 <https://reviews.llvm.org/D54696>) back into this patch, so that legalization support is complete and this can stand on its own.
- For soft float legalization, I would switch from libcall legalization to expanding and recursively legalizing. Adding so many new compiler-rt functions is an unnecessary burden.
- Finally, something worth mentioning is that the legalization implemented here is not compatible with trapping fptoi (at least one of the expansions isn't). I don't believe trapping fptoi's are actually legal per langref, but I've also seen people adjust x86 fptoi lowering to work with trapping fptoi at some point, so I'm a bit confused on what the state here is.


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

https://reviews.llvm.org/D54749