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

Tweak abstract machine reference table #188

Merged
merged 2 commits into from
Jun 18, 2023

Conversation

sogaiu
Copy link
Contributor

@sogaiu sogaiu commented Jun 17, 2023

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

Specifically the changes are:

  • Added missing dest argument for prop
  • Swapped the last two arguments of puti
  • Changed spelling of tcheck to tchck

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
Copy link
Contributor Author

sogaiu commented Jun 17, 2023

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 sogaiu marked this pull request as draft June 17, 2023 10:50
@sogaiu sogaiu marked this pull request as ready for review June 17, 2023 10:59
@sogaiu
Copy link
Contributor Author

sogaiu commented Jun 18, 2023

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.

@bakpakin bakpakin merged commit 614e026 into janet-lang:master Jun 18, 2023
1 check passed
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

2 participants