Make MaxCombinedBinarySize configurable#7820
Conversation
yamt
force-pushed
the
max-combined-size
branch
from
67ed9a6 to
43902cc
Compare
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Makes sense to add, sounds good.
You will need to run ./scripts/update_help_checks.py to update the help expecations for the new flag.
Also please add a test for this, something like setting a very low max size that prevents an inlining. For an example test, see test/lit/passes/inlining-const-args.wat - like there, the test can use RUN with several different flag values, to show the different results.
src/passes/Inlining.cpp
Outdated
| Name inlinedName = inlinedFunction->name; | ||
| if (!isUnderSizeLimit(func->name, inlinedName)) { | ||
| if (!isUnderSizeLimit( | ||
| func->name, inlinedName, getPassRunner()->options)) { |
There was a problem hiding this comment.
It might be simpler to not pass in the options, and call getPassRunner() in the called function?
There was a problem hiding this comment.
yes. i reverted this part. thank you.
yamt
force-pushed
the
max-combined-size
branch
from
43902cc to
c54184f
Compare
thank you. i added a test. |
yamt
force-pushed
the
max-combined-size
branch
from
c54184f to
8e5d57c
Compare
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Looks good aside from minor comments on the test.
| @@ -0,0 +1,735 @@ | |||
| ;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. | |||
There was a problem hiding this comment.
The name of the testcase has a typo:
test/lit/passes/inlining-max-comibned-size.wat
should be
test/lit/passes/inlining-max-combined-size.wat
| ;; RUN: foreach %s %t wasm-opt -O3 -S -o - | filecheck %s --check-prefix=default | ||
| ;; RUN: foreach %s %t wasm-opt -O3 --inline-max-combined-binary-size=96 -S -o - | filecheck %s --check-prefix=imcbs96 | ||
| ;; RUN: foreach %s %t wasm-opt -O3 --inline-max-combined-binary-size=90 -S -o - | filecheck %s --check-prefix=imcbs90 | ||
| ;; RUN: foreach %s %t wasm-opt -O3 --inline-max-combined-binary-size=0 -S -o - | filecheck %s --check-prefix=imcbs0 |
There was a problem hiding this comment.
Check-prefixes should be in all-caps, just as a convention, so IMCBS0 etc.
| ;; RUN: foreach %s %t wasm-opt -O3 --inline-max-combined-binary-size=0 -S -o - | filecheck %s --check-prefix=imcbs0 | ||
|
|
||
| (module | ||
| ;; small: (type $0 (func (param i32 i32 i32))) |
There was a problem hiding this comment.
is small leftover from an earlier version perhaps? I don't see a RUN line for it.
There was a problem hiding this comment.
oops. fixed. thank you.
| ;; RUN: foreach %s %t wasm-opt -O3 --inline-max-combined-binary-size=90 -S -o - | filecheck %s --check-prefix=imcbs90 | ||
| ;; RUN: foreach %s %t wasm-opt -O3 --inline-max-combined-binary-size=0 -S -o - | filecheck %s --check-prefix=imcbs0 | ||
|
|
||
| (module |
There was a problem hiding this comment.
Please add a toplevel comment explaining what to look for here. Something like: "Functions X,Y get inlined in IMCBS0 and Z but not W, because then the size would [..]" etc. With such comments in the tests, it's easy to understand what is being tested.
yamt
force-pushed
the
max-combined-size
branch
2 times, most recently
from
e2fbee5 to
bd73d83
Compare
Use case: I want to avoid generating "huge" (eg. 50KB) functions, which can involve certain kind of text relocations in aot-compiled binaries for my target. (WAMR, xtensa, l32r+jx target address)
yamt
force-pushed
the
max-combined-size
branch
from
bd73d83 to
2880f9c
Compare
2 participants
Use case: I want to avoid generating "huge" (eg. 50KB) functions,
which can involve certain kind of text relocations in aot-compiled
binaries for my target. (WAMR, xtensa, l32r+jx target address)