chore: backend tweaks #4890
chore: backend tweaks #4890
Conversation
| @@ -2540,7 +2540,7 @@ module Closure = struct | |||
| I32Type :: Lib.List.make n_args I32Type, | |||
| FakeMultiVal.ty (Lib.List.make n_res I32Type))) in | |||
| (* get the table index *) | |||
| Tagged.load_forwarding_pointer env ^^ | |||
| (*Tagged.load_forwarding_pointer env ^^ FIXME: NOT needed, accessing immut slots*) | |||
There was a problem hiding this comment.
Did @luc-blaeser approve this? Seems a bit risky to me.
There was a problem hiding this comment.
Hmm, maybe that's ok because its only accessing the fun ptr field of the closure which is just an immutable field.
There was a problem hiding this comment.
I don't know how Luc wants to implement named (shared) functions, maybe then the index changes? Better ask!
There was a problem hiding this comment.
Please note that targets of immutable slots can also be moved by GC, so this extra step is necessary I believe
| @@ -2540,7 +2540,7 @@ module Closure = struct | |||
| I32Type :: Lib.List.make n_args I32Type, | |||
| FakeMultiVal.ty (Lib.List.make n_res I32Type))) in | |||
| (* get the table index *) | |||
| Tagged.load_forwarding_pointer env ^^ | |||
| (*Tagged.load_forwarding_pointer env ^^ FIXME: NOT needed, accessing immut slots*) | |||
There was a problem hiding this comment.
there can never be a mutation of the funptr_field between old space and newspace. Maybe we should have Tagged.load_immutable_field (= Tagged.load_field) to signify that load_forwarding_pointer is not needed.
| @@ -5520,7 +5520,7 @@ module IC = struct | |||
| | Flags.(ICMode | RefMode) -> | |||
| system_call env "call_cycles_add128" | |||
| | _ -> | |||
| E.trap_with env "cannot accept cycles when running locally" | |||
| E.trap_with env "cannot add cycles when running locally" | |||
| @@ -10937,9 +10936,8 @@ and compile_prim_invocation (env : E.t) ae p es at = | |||
|
|
|||
| StackRep.of_arity return_arity, | |||
| code1 ^^ StackRep.adjust env fun_sr SR.Vanilla ^^ | |||
| Closure.prepare_closure_call env ^^ (* FIXME: move to front elsewhere too *) | |||
There was a problem hiding this comment.
Some fields (that could have mutable portions) could be in newspace, so let's do this eagerly. TODO: spot other similar uses...
N.B. Line 10943 potentially looked into oldspace, but probably only loaded an immutable field (function index?).
| @@ -10937,9 +10936,8 @@ and compile_prim_invocation (env : E.t) ae p es at = | |||
|
|
|||
| StackRep.of_arity return_arity, | |||
| code1 ^^ StackRep.adjust env fun_sr SR.Vanilla ^^ | |||
| Closure.prepare_closure_call env ^^ (* FIXME: move to front elsewhere too *) | |||
There was a problem hiding this comment.
So this now storing the prepared (forwarded closure in clos and re_loading it later to pass to call_closure (already forwarded). I guess that's ok. @luc-blaeser ?
There was a problem hiding this comment.
Yeah, this won't save an instruction, but following the recent pointer in both cases sounds like a good idea (less cache misses :-)
| @@ -2332,7 +2330,7 @@ module Closure = struct | |||
| I64Type :: Lib.List.make n_args I64Type, | |||
| FakeMultiVal.ty (Lib.List.make n_res I64Type))) in | |||
| (* get the table index *) | |||
| Tagged.load_forwarding_pointer env ^^ | |||
| (*Tagged.load_forwarding_pointer env ^^ FIXME: NOT needed, accessing immut slots*) | |||
There was a problem hiding this comment.
Targets of immutable slots may also be moved by the GC.
Broken out of #4608. Mostly harmless cleanups.