janet-lang / janet-lang.org

Website for janet
https://janet-lang.org
MIT License
90 stars 59 forks source link

Tweak abstract machine reference table #188

Closed sogaiu closed 1 year ago

sogaiu commented 1 year ago

This PR contains some suggestions for the abstract machine reference table.

Specifically the changes are:

On a side note, regarding the argument swapping for puti -- the order was surprising to me given the pattern for get, geti and put. I looked a bit at some history for it and it seems to have been that way for a long time. Curious about the order.

Any hints?

sogaiu commented 1 year ago

I'm still trying to clarify my understanding of ldu and setu, but is it possible that the info in the table is slightly off for setu as well?

Currently, vm.c has the following for each:

    VM_OP(JOP_LOAD_UPVALUE) {
        int32_t eindex = B;
        int32_t vindex = C;
        JanetFuncEnv *env;
        vm_assert(func->def->environments_length > eindex, "invalid upvalue environment");
        env = func->envs[eindex];
        vm_assert(env->length > vindex, "invalid upvalue index");
        vm_assert(janet_env_valid(env), "invalid upvalue environment");
        if (env->offset > 0) {
            /* On stack */
            stack[A] = env->as.fiber->data[env->offset + vindex];
        } else {
            /* Off stack */
            stack[A] = env->as.values[vindex];
        }
        vm_pcnext();
    }

    VM_OP(JOP_SET_UPVALUE) {
        int32_t eindex = B;
        int32_t vindex = C;
        JanetFuncEnv *env;
        vm_assert(func->def->environments_length > eindex, "invalid upvalue environment");
        env = func->envs[eindex];
        vm_assert(env->length > vindex, "invalid upvalue index");
        vm_assert(janet_env_valid(env), "invalid upvalue environment");
        if (env->offset > 0) {
            env->as.fiber->data[env->offset + vindex] = stack[A];
        } else {
            env->as.values[vindex] = stack[A];
        }
        vm_pcnext();
    }

That suggests to me that the info in the table (the middle column):

ldu | (ldu dest env index) | $dest = envs[env][index]

and:

setu | (setu env index val) | envs[env][index] = $val

should be more symmetric.

That is, setu should be called like (setu val env index).

Yet to understand the sample code I have that uses setu well enough -- so I'm not quite sure about the assertion above (^^;

This is what I've got:

(defn outer
  [o-arg]
  (var o-var 0)
  (defn inner [x] (set o-var x))
  (inner 3))

disasm gives me something like:

 {:arity 1 
  :bytecode @[(ldi 2 0) (clo 3 0) (movn 4 3) (ldi 5 3) (push 5) (tcall 4)] 
  :constants @[] 
  :defs @[{:arity 1 
           :bytecode @[(setu 0 0 2) (ldu 2 0 2) (ret 2)] 
           :constants @[] 
           :defs @[] 
           :environments @[-1] 
           :max-arity 1 :min-arity 1 
           :name "inner-2" 
           :slotcount 3 
           :source "repl" 
           :sourcemap @[(22 21) (22 21) (22 21)] 
           :structarg false 
           :symbolmap @[(:upvalue 0 0 o-arg) 
                        (:upvalue 0 1 outer-2) 
                        (:upvalue 0 2 o-var) 
                        (0 3 0 x) 
                        (0 3 1 inner-2)] 
           :vararg false}] 
  :environments @[] 
  :max-arity 1 :min-arity 1 
  :name "outer-2" 
  :slotcount 6 
  :source "repl" 
  :sourcemap @[(21 3) (22 3) (22 3) (23 3) (23 3) (23 3)] 
  :structarg false 
  :symbolmap @[(0 6 0 o-arg) 
               (0 6 1 outer-2) 
               (0 6 2 o-var) 
               (2 6 4 inner-2)] 
  :vararg false}
sogaiu commented 1 year ago

My current understanding is that setu's signature is:

(setu val env index)

so the table's current content:

(setu env index val)

could benefit from some adjustment.

Thus, the additional change in c779fdd seems warranted to me.