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

Function References Draft #168

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

iainsmith
Copy link
Contributor

@iainsmith iainsmith commented Jan 4, 2025

Function References Overview Link

👋 I started looking at adding support for the function-references extension. This is very much a work in progress, that I'd appreciate feedback on. I'm still building up context on the codebase, so I suspect I'm missing several fairly obvious issues. If you do check it out locally, feel free to push improvements directly to my branch, and I'll pull them in.

So far:

  • Updated WatParser to support (ref null? $variable) syntax
  • Configure spec tests to run the wast tests for function-references & GC
    • Make SpectestResult top level so that we can track partially implemented proposals.
  • Added an instruction for call_ref (that's not passing the call_ref.wast tests yet)

TODO:

  • Get call_ref implemented correctly (and stop leaving references on the stack)
  • Add remaining instructions
  • Get all the Function reference wast test passing
  • Check the benchmarks

Questions:

I punted on separating out a HeapType struct/enum for now, as I wasn't sure what shape it would land up in, and how big the knock on change would be. Do you have a shape in mind for a HeapType or shall I keep going with the extra cases for ReferenceType for now?

I'm hoping to have more time to work on this, but it will depend on other commitments.


I've got 8/34 of the initial call_ref.wast tests passing, while the rest are failing with Expected i32 on the stack top but got ref(WasmTypes.ReferenceType.funcRef) . Run testFunctionReferencesProposals if you want to reproduce it locally.

@iainsmith iainsmith marked this pull request as draft January 4, 2025 01:34
@kateinoigakukun
Copy link
Member

Thanks @iainsmith!

I punted on separating out a HeapType struct/enum for now, as I wasn't sure what shape it would land up in, and how big the knock on change would be. Do you have a shape in mind for a HeapType or shall I keep going with the extra cases for ReferenceType for now?

I would suggest following the structure used in the spec like:

/// Reference types
public struct ReferenceType: Equatable, Hashable {
    public var isNullable: Bool
    public var heapType: HeapType

    public static var funcRef: ReferenceType {
        ReferenceType(isNullable: true, heapType: .func)
    }

    public static var externRef: ReferenceType {
        ReferenceType(isNullable: true, heapType: .func)
    }

    public init(isNullable: Bool, heapType: HeapType) {
        self.isNullable = isNullable
        self.heapType = heapType
    }
}

public enum HeapType: Equatable, Hashable {
    /// A reference to any kind of function.
    case `func`
    /// An external host data.
    case extern
}

@iainsmith iainsmith force-pushed the iain/reference-proposal branch from 65c9324 to 9141418 Compare January 4, 2025 11:43
@kateinoigakukun
Copy link
Member

I added tail-call support in the main branch because function-ref proposal depends on the proposal.

It might also be a good example to see how to implement a new proposal in WasmKit.

@iainsmith iainsmith force-pushed the iain/reference-proposal branch from 9141418 to 494e878 Compare January 4, 2025 18:51
Comment on lines 169 to 170
// TODO: Enable smoke check for encoding
// _ = try wat.encode()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to merge this branch once the WAT/WasmParser implementation passes the Spectest suite. I don't have enough time to work on it this week, so I won't push any further commits, but feel free to ask any questions you may have.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kateinoigakukun, will look at the commits you pushed and try to focus on the Wat/WasmParser.

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.

2 participants