Any more feedback on this?

Fixed typos.

Fix clang-format warnings.

Fix clang-format warnings.

Now only uses FP_TO_INT_SAT nodes for lowering. The nodes are selected in isel.

Fix typo.

Using Type::getFloatingPointTy now.

Use default expansion for f128 conversions; they aren't native so we can't be sure that they saturate.

Removed NativeSaturation parameter and made all lowering happen in Lower.

Redid lowering.

Followup to removing vector expansion in the parent patch.

Removed vector expansion.

Fixed failing test cases.

In D86078#2260930, @paulwalker-arm wrote:

I would have experimented a little to understand better how things work, but I'm guessing the patch is based on other work because I couldn't some of the affected functions/nodes.

Updated method name.

Addressed comments.

Removed superfluous NaN check and updated tests.

I don't know why the premerge tests are failing. I don't see those failures locally, so it's rather hard to debug it, especially since the premerge build lacks proper debug info.

Rebased.

Rebased.

Rebased.

Rebased.

Rebased.

Rebased.

Added promotion mechanism.

Added a promotion mechanism to handle cases where the floating point type cannot be used to rescale the value.

In D85961#2255847, @rjmccall wrote:

Would it be completely unthinkable to "promote" calculations to a larger FP type (both here and in codegen) if the exponent bits are insufficient to hold the necessary scaling?

You could have the same problem with float/bfloat and a 128-bit fixed-point type, right? 128-bit integer types aren't at all ridiculous. I think this is something you should accommodate properly in your design; I don't think it's that problematic.

In D62574#2242471, @Anastasia wrote:

The only thing is that it would be good to test the new target setting logic somehow... do you have any ideas in mind? We could think of creating a dummy target for that or adding a dummy setting in existing targets - maybe SPIR could be a candidate. We have done something similar in the past if you look at FakeAddrSpaceMap in LangOpts.

Made natively saturating expansions use integer comparisons instead.

In D54749#2242802, @efriedma wrote:

At least for the IEEE formats, you should be able to just destructure the bit-pattern of the float, right? Normalize and do some extends and shifts.

If we're going down that path, maybe simpler to just mess with the float scaling factor. If a float is close to the largest finite float, in IEEE formats, the fractional part must be zero. So we can do the float-to-int conversion on the unscaled float, and scale the resulting integer. Probably faster than trying to explicitly destructure a float.

Addressed comments.

I hadn't considered half precision as that's sort of off my radar. That does make both these conversion methods and the corresponding codegen implementation rather problematic.

In D54749#2239801, @rjmccall wrote:

I can't competently review LLVM code-gen patches, but I can review intrinsic design. If the intended use case here is fixed point, should there be a scale parameter rather than expecting the caller to scale the input? Or is scaling the input basically the expected implementation on all hardware? Scaling the input can be lossy if we overflow to infinity, so that would be making a tacit assumption here that if that happens, the original value must've been outside the representation range of the integer. For float (and bfloat), the exponent goes up to 127, so you would need to be converting to a pretty large fixed-point format to have problems with lossy overflow. For half, the exponent only goes up to 16, so converting to a 32-bit fixed-point format with a scale could overflow, and you probably won't be able to use these intrinsics.

Fix comment mistake.

This needs a few tests.

Rebased and did minor adjustments.

In D54749#2238740, @arsenm wrote:

Missing globalisel support

Ping.

In D86078#2233560, @paulwalker-arm wrote:

Given you are making use of the fact that AArch64 fp_to_int already does the saturation part, I'm wondering if the different-width saturation can be moved from the input to the output? Specially I'm thinking this will result in nicer code as the min/max immediate values will be easier to produce in integer form.

@gousemoodhin I just want to check; are you working on or planning to work on float-fixed support in the near future? Floating point conversions are the next step for the fixed-point support, so if you aren't immediately looking at it, I would like to take a shot at it.

Added comments and more tests.

Revamped patch. It's now based on the FixedPointBuilder.

In D85314#2224845, @rjmccall wrote:

I'm assuming that you've extracted this code out of the corresponded Clang code and that there's nothing functionally wrong with it. The interface looks fine. Can we add LLVM tests for this, or that too difficult to fit into the existing test infrastructure, and we should just wait to test it by moving Clang to use it?

Amended comment.

Addressed comment.

Adding some more reviewers to get some more eyes on this.

Fixed minor formatting nits.

Updated summary on the second operand of the node.

It doesn't feel like this patch got a very positive reception, but I'd still like to try a bit more to get it in.

Added IntrWillReturn.

Addressed comments.

Addressed comments.

Removed stray merge mistake.

Rebased and addressed comments.

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

Thanks for reviewing! I'm not really sure who to add as reviewers for D85314, though, and no one has replied to the RFC on the mailing list yet.

I think this works now. It should probably be given a few more reviewers to have a look. Do you have some suggestions, @Anastasia ?

Rebased and updated summary.