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

Mon Aug 17 01:00:40 PDT 2020

ebevhan reclaimed this revision.
ebevhan added a comment.

In D54749#2218465 <https://reviews.llvm.org/D54749#2218465>, @nikic wrote:

> 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.

All right.

> - 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.

Well, the value on the fixed point nodes isn't for saturation, and I think there are actually a few instances in which the fixed point nodes (such as division) could have used a saturation width for simplifying legalization. I didn't add it, though. It's very convenient for these nodes.

> - 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.

Hm, okay. I actually did originally fold both of those into this patch but then took it out since it wasn't originally part of it.

I agree on the libcalls. I was also planning on doing the expansion directly instead of going for libcalls. In most cases it shouldn't be that much worse than if we had a direct libcall. The worst case is 4 libcalls, but I think that on targets that do not support floating point, it should be expected to be slow anyway.

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

https://reviews.llvm.org/D54749