Arm backend: Size Test for Cortex-M #9569
Conversation
digantdesai
requested review from
larryliu0820,
kirklandsign,
JacobSzwejbka,
lucylq,
swolchok and
manuelcandales
as code owners
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/9569
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit af648de with merge base b66c319 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
the
CLA Signed
digantdesai
added
ciflow/trunk
topic: not user facing
and removed
CLA Signed
facebook-github-bot
added
the
CLA Signed
digantdesai
force-pushed
the
size_test_m
branch
from
364c2d6 to
e30bc7d
Compare
.github/scripts/run_nm.py
Outdated
| sys.exit(1) | ||
|
|
||
| def _get_nm_output(self) -> Dict[str, Symbol]: | ||
| args = ["--print-size", "--size-sort", "--reverse-sort", "--demangle"] |
There was a problem hiding this comment.
you might want to specify the format. it can differ between systems.
.github/scripts/run_nm.py
Outdated
| symbol_pattern = re.compile( | ||
| r"(?P<addr>[0-9a-fA-F]+)\s+(?P<size>[0-9a-fA-F]+)\s+(?P<type>\w)\s+(?P<name>.+)" | ||
| ) | ||
| match = symbol_pattern.match(line) |
There was a problem hiding this comment.
as written, compiling this regex doesn't help. you'd need to store it somewhere that outlives a single call to this function.
There was a problem hiding this comment.
fixed by moving it up out of the function. Thanks.
| @@ -0,0 +1,167 @@ | |||
| # Copyright (c) Meta Platforms, Inc. and affiliates. | |||
There was a problem hiding this comment.
perhaps we could use bloaty?
There was a problem hiding this comment.
yeah I did think about it. This is just nm TBH so not sure if we need it to be more complicated than that. Wrote a python wrapper to print sorted table, and use cross-tools. But bloaty is nice though a bit "bloated" with dependency etc.
digantdesai
force-pushed
the
size_test_m
branch
7 times, most recently
from
0ffc0cb to
25c5dea
Compare
Could not find ET installation when cross-compiling.
.github/scripts/run_nm.py
Outdated
| self.filter is None | ||
| or self.filter not in ["all", "executorch", "executorch_text"] |
There was a problem hiding this comment.
nit: isn't the None check redundant?
There was a problem hiding this comment.
you could make this easier to maintain and extend by creating a FILTER_NAME_TO_FILTER_AND_LABEL table:
def is_executorch_symbol(s):
return "executorch" in s.name or s.name.startswith("et")
FILTER_NAME_TO_FILTER_AND_LABEL = {
"all": (None, "All"),
"executorch": (is_executorch_symbol, "ExecuTorch"),
"executorch_text": (lambda s: is_executorch_symbol(s) and s.symbol_type.lower() == "t", "ExecuTorch .text")
}
then access it as
filter, label = FILTER_NAME_TO_FILTER_AND_LABEL.get(self.filter, FILTER_NAME_TO_FILTER_AND_LABEL["all"])
print_table(filter, label)
and use FILTER_NAME_TO_FILTER_AND_LABEL.keys() in the argument definition below
Prints useful tables about binary size. TODO: expand it to parse linker map and also dump module, objects, and section names with a given symbol.
Just getting started. We will build on top of this.
| required=False, | ||
| default="all", | ||
| help="Filter symbols by pre-defined filters", | ||
| choices=["all", "executorch", "executorch_text"], |
There was a problem hiding this comment.
this should be FILTER_NAME_TO_FILTER_AND_LABEL.keys() (wrapped in list() if we must)
### Summary Add size test for cortex-M and add it in the CI
Summary
Add size test for cortex-M and add it in the CI