-
Notifications
You must be signed in to change notification settings - Fork 836
[CodeAnnotations] Implement function-level inlining hints #8265
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
base: main
Are you sure you want to change the base?
Conversation
| auto offset = exprOffset - funcDeclarationsOffset; | ||
| // Compute the offset: it should be relative to the start of the | ||
| // function locals (i.e. the function declarations). | ||
| offset = exprOffset - funcDeclarationsOffset; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this expression offset ever end up being 0 as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can't be 0 - the function has something at offset 0 (the number of locals, even if has no locals, that will take a byte for "0"). This is why we decided it was safe to use offset 0 in the spec.
test/lit/inline-hints-func.wast
Outdated
| ;; RUN: wasm-opt -all %s -S -o - | filecheck %s | ||
| ;; RUN: wasm-opt -all --roundtrip %s -S -o - | filecheck %s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move the RUN lines to the top of the file for consistency with other tests? I was about to leave a comment asking how it was possible that there were no RUN lines here before I found them 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, moved.
| (@metadata.code.inline "\12") | ||
| (func $func-annotation | ||
| ;; The annotation here is on the function. | ||
| (drop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to my question about the offsets, can we properly roundtrip an annotation on the first expression in a function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and we test that, see e.g.
binaryen/test/lit/inline-hints.wast
Line 43 in 9acdd65
| (@metadata.code.inline "\00") |
This is the first function-level annotation, so this includes some
necessary fixes for that.
Diff without whitespace is smaller.