replace checked_fptosi intrinsics with Julia implementation

[simonbyrne]

Fixes #14549.

As @yuyichao points out, this will effect inlining rules.

[yuyichao]

Let's check =) runbenchmarks(ALL, vs = "JuliaLang/julia:master")

[simonbyrne]

@nanosoldier runbenchmarks(ALL, vs=":master")

[simonbyrne]

This has been updated to do an explicit range check, so should no longer utilise any undefined behaviour

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

[simonbyrne]

Hmm, the slowdown seems to be due to the fact that the length of FloatRange and LinSpace is a floating point number, which needs to be converted a lot.

[tkelman]

was there going to be a comment here then changed your mind?

[simonbyrne]

I can't remember, this was quite a long time ago (if only I had left a comment on what that comment was going to be...)

[simonbyrne]

@nanosoldier runbenchmarks(ALL, vs=":master")

[jrevels]

@simonbyrne looks like you may have submitted the job with the wrong syntax, and then edited to the correct syntax? Nanosoldier ignores comment edits to prevent accidental job resubmission. Triggering it with a new comment should work:

@nanosoldier runbenchmarks(ALL, vs=":master")

[simonbyrne]

thanks!

[vchuravy]

Would this also work for Float16?

[simonbyrne]

It should do, the logic is pretty much the same.

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

[simonbyrne]

Actually, it won't work for Float16, but I've added a note as to why. I didn't include the Float16 logic here, as the current method (converting to Float32 first) is probably more efficient.

[simonbyrne]

Updated. It seems like the change in performance is a bit of a wash. We may want to look at speeding up FloatRange and LinSpace either by inlining or perhaps even redesigning so the length is stored as an integer, but this is probably better in a different PR.

Any objections to merging? cc @ViralBShah @vchuravy

[vchuravy]

No objections to merging.

I would still be interested in a implementation for Float16. Mostly because I am looking at improving our support for Float16 on platforms that support them natively.

[simonbyrne]

It should be possible via an extra branch: just check if Tf(typemin(Ti)) == Inf, in which case use an inequality rather than equality for the lower bound.

[tkelman]

that's a pretty bad slowdown in sparse indexing, worth looking into and profiling the difference

[simonbyrne]

I couldn't figure it out: there don't appear to be any conversion calls, and I can't recreate it locally.

One more time:

@nanosoldier runbenchmarks(ALL, vs=":master")

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

[simonbyrne]

The sparse issue seems to have mostly gone.

[simonbyrne]

Given the inconsistency, it seems that the regressions are mostly noise. We do get a nice speed boost on some though. Shall we merge this?

[musm]

Out of curiosity, how reliable is nanosoldier if +-15% can be mainly attributed to noise?

[simonbyrne]

@nanosoldier runbenchmarks(ALL, vs=":master")

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

[simonbyrne]

Okay, we'll have to figure out how to improve FloatRange and LinSpace indexing, but I think this is good to go.

[simonbyrne]

Will probably want to backport this to get tests to pass on ARM/Power in next release.

[tkelman]

jl_checked_fptoui and jl_checked_fptosi were exported from libjulia, but doesn't look like any packages call into them