ZJIT: Remove a bunch of Option from codegen #14238
Conversation
| let ep = gen_get_ep(asm, level); | ||
| let offset = -(SIZEOF_VALUE_I32 * i32::try_from(local_ep_offset).ok()?); |
There was a problem hiding this comment.
This is one of the few cases I am uncertain about. Maybe we should catch these cases in the caller for a clearer signal
| let ep = gen_get_ep(asm, level); | ||
| match val { | ||
| // If we're writing a constant, non-heap VALUE, do a raw memory write without | ||
| // running write barrier. | ||
| lir::Opnd::Value(const_val) if const_val.special_const_p() => { | ||
| let offset = -(SIZEOF_VALUE_I32 * i32::try_from(local_ep_offset).ok()?); | ||
| let offset = -(SIZEOF_VALUE_I32 * i32::try_from(local_ep_offset).unwrap()); |
There was a problem hiding this comment.
This is one of the few cases I am uncertain about. Maybe we should catch these cases in the caller for a clearer signal
| asm.mov(Opnd::mem(64, ep, offset), val); | ||
| } | ||
| // We're potentially writing a reference to an IMEMO/env object, | ||
| // so take care of the write barrier with a function. | ||
| _ => { | ||
| let local_index = c_int::try_from(local_ep_offset).ok().and_then(|idx| idx.checked_mul(-1))?; | ||
| let local_index = c_int::try_from(local_ep_offset).ok().and_then(|idx| idx.checked_mul(-1)).unwrap(); |
There was a problem hiding this comment.
This is one of the few cases I am uncertain about. Maybe we should catch these cases in the caller for a clearer signal
There was a problem hiding this comment.
Could we include the value of local_ep_offset in the panic message then?
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
We have a verifier that runs in debug mode that should prevent this. Simplify a bunch of call sites.
They don't need it anymore; jit.get_opnd doesn't return Option anymore.
This reverts commit 1b67940.
cc75c3f to
b4bc4b6
Compare
b4bc4b6 to
dd8a496
Compare
|
Opening this up for review |
|
|
| @@ -439,6 +439,7 @@ pub enum SideExitReason { | |||
| CalleeSideExit, | |||
| ObjToStringFallback, | |||
| UnknownSpecialVariable(u64), | |||
| TemporaryBug, | |||
There was a problem hiding this comment.
I get that you want to leave the bug only temporarily, but it's always nice to describe what it is. Could we call it UnknownDefinedType(op) (also I feel like they are not "Unknown" to us but just "Unhandled" btw) or DefinedMethod?
I want to reduce the chance of accidentally having a not-implemented or
internal-compiler-error related side-exit after some code for an instruction
has already been emitted. So in this PR I make some of these ICEs cause a noisy
error and remove the spurious use of
Options.