Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JIT: Adding multiply and division for int type + mixing float & int type #2293

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jviereck
Copy link

No description provided.

self.add_binary_operator_float(op, fa, b.val)
}
(JitType::Float, JitType::Int) => {
let fb = self.builder.ins().fcvt_from_sint(types::F64, b.val);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if b.val value doesn't fit in f64? this is OverflowError case without jit.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, this is not handled yet. Note that also the division by zero is not handled by the jit yet. I was not sure how to implement exception throwing for such cases and left it therefore out.

Can you give me a pointer how to throw an exception from the JIT code?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think division by zero can be handled in f64/f64 operation because it is part of them (though it is not supported yet). But the overflow error is out of f64 operation so need to be handled before going to the f64 operation step.

Unfortunately, I don't know that much about JIT yet either.
@Skinny121 @coolreader18 Do you have any hint for that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's possible to throw an exception yet; right now I think we just trap on overflow, without any sort of recovery

Copy link
Contributor

@afonso360 afonso360 Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 Hey, I was pointed here from #4269.

Cranelift's fcvt_from_sint never traps! What happens instead is loss of precision.

I've built a small CLIF (Cranelift IR) program to test this out.
test run
target x86_64

function %test(i64) -> f64 {
block0(v0: i64):
    v1 = fcvt_from_sint.f64 v0
    return v1
}

; run: %test(1) == 0x1.0
; run: %test(2) == 0x2.0
; run: %test(9223372036854775807) == 0x1.0000000000000p63
; run: %test(9223372036854775806) == 0x1.0000000000000p63
; run: %test(9223372036854775805) == 0x1.0000000000000p63

These are the results when compiled via Cranelift! Ignore the weird hexadecimal notation for floats. The point is that at the limits of i64 the resulting float value is the same. In practice there are a bunch of other int values that map to the same float value.


And as far as I understand it behaves similarly to what python does:

>>> float(9223372036854775808)
9.223372036854776e+18
>>> float(9223372036854775807)
9.223372036854776e+18
>>> float(9223372036854775806)
9.223372036854776e+18

We currently can't represent values larger than a i64 in the JIT, and they may have to be handled specially, so we would probably have to lower them in a different way.

If we need to handle a BigInt sort of type, then yes we do need to worry about that situation and we could probably detect that in the code that does int to float conversions.

On a more general note about exceptions, I think the current approach is to let traps be raised, and then eventually catch them and convert them into exceptions. At least that seems to be what is implied by this code.


Going the other way, Float to Integer does raise a trap, and we probably need to handle that if we come across that situation! But I don't think that situation applies here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants