webassemblyjs icon indicating copy to clipboard operation
webassemblyjs copied to clipboard

Move local variables onto function node in AST

Open maurobringolf opened this issue 7 years ago • 4 comments

Currently we store them as instructions in the function body which is closer to text format, but conceptually not quite right since they're not instructions. I think we should move them into a property of the function node and remove them from the body.

maurobringolf avatar May 15 '18 07:05 maurobringolf

I have mixed feelings on that.

Here are a few points:

  • Locals can only be before any instructions in a func body.
  • Allocation is done by going through the local instructions.
  • func params are also consider as locals. And currently part of our func signature.
  • the Function Instance doesn't have the locals and at the interpreter level we only have a Func instance.

xtuc avatar May 15 '18 10:05 xtuc

My motivation for this was that in #306, in the type checker and conceptually it seems more natural to have locals as a property of the function node. I just assumed it would be similar for the interpreter but have not looked into it. This does not take into account the fact that params are locals though, so it does not make a lot of sense as is I agree.

maurobringolf avatar May 15 '18 11:05 maurobringolf

Currently we store them as instructions in the function body which is closer to text format

Well ... it does depend on how you format the text file.

This implies that the local is part of the function body ...

(func (param i32) (param f32)
  (local f64)
  (nop)
)

But using that argument, are parameters part of the function body too?

(func
  (param i32)
  (param f32)
  (local f64)
  (nop)
)

No, of course not.

Mentally, I see it like this:

(func (param i32) (param f32) (local f64)
  (nop)
)

And would agree, locals are better placed within the Func AST node.

ColinEberhardt avatar May 15 '18 14:05 ColinEberhardt

Ok that's a good point.

Here the code handling the locals in the interpreter: https://github.com/xtuc/webassemblyjs/blob/master/packages/webassemblyjs/src/interpreter/kernel/exec.js#L693-L698

Quick clarification, function locals end up in stackframe locals. We pass the params of the function into the stack frame here https://github.com/xtuc/webassemblyjs/blob/master/packages/webassemblyjs/src/interpreter/host-func.js#L91.

At this point we don't have the func Node anymore (or in another form), we have a funcinstance, which according to the specification doesn't have the locals.

xtuc avatar May 15 '18 15:05 xtuc