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

[Parser] Parse ref.func #6097

Merged
merged 2 commits into from
Nov 15, 2023
Merged

[Parser] Parse ref.func #6097

merged 2 commits into from
Nov 15, 2023

Conversation

tlively
Copy link
Member

@tlively tlively commented Nov 9, 2023

No description provided.

@tlively tlively requested a review from ashleynh November 9, 2023 22:57
@@ -839,7 +839,10 @@ Result<> IRBuilder::makeRefIsNull() {
return Ok{};
}

// Result<> IRBuilder::makeRefFunc() {}
Result<> IRBuilder::makeRefFunc(Name func) {
push(builder.makeRefFunc(func, wasm.getFunction(func)->type));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider adding a comment about wasm-ir-builder having a stricter makeRefFunc() than wasm-builder, one that will throw an exception if the function being made reference to is not already added to the module.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add a general comment about this to the top-level IRBuilder doc comment because this applies to many functions.

(func $ref-func
ref.func $ref-func
drop
ref.func 102
Copy link
Collaborator

@ashleynh ashleynh Nov 13, 2023

Choose a reason for hiding this comment

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

How did you know $ref-func was at function index 102?

Copy link
Member Author

Choose a reason for hiding this comment

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

trial and error ;)

@@ -388,9 +388,8 @@ struct NullInstrParserCtx {
return Ok{};
}
Result<> makeRefIsNull(Index) { return Ok{}; }

Result<> makeRefFunc(Index, FuncIdxT) { return Ok{}; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the NullInstrParserCtx for?

Copy link
Member Author

Choose a reason for hiding this comment

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

The first few phases of parsing only care about finding top-level module elements like globals, functions, etc, or parsing types, so they don't need to do anything interesting with the instructions. The parser contexts for those phases inherit from NullInstrParserCtx to avoid having to repeat this "do nothing" behavior for each of them separately.

@tlively
Copy link
Member Author

tlively commented Nov 15, 2023

Merge activity

  • Nov 14, 7:40 PM: @tlively started a stack merge that includes this pull request via Graphite.
  • Nov 14, 7:40 PM: @tlively merged this pull request with Graphite.

@tlively tlively merged commit 9846aab into main Nov 15, 2023
20 of 26 checks passed
@tlively tlively deleted the parser-ref-func branch November 15, 2023 00:40
radekdoulik pushed a commit to dotnet/binaryen that referenced this pull request Jul 12, 2024
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