Arm backend: Make warning statement more clear#18582
Arm backend: Make warning statement more clear#18582gggekov wants to merge 1 commit intopytorch:mainfrom
Conversation
Signed-off-by: George Gekov <george.gekov@arm.com> Change-Id: I83fead0e62a0cf012b6754ba572efe4e2deb8b50
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/18582
Note: Links to docs will display an error until the docs builds have been completed. This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
bot
added
the
CLA Signed
gggekov
added
ciflow/trunk
partner: arm
This PR needs a
|
There was a problem hiding this comment.
Pull request overview
Updates the Arm FVP runner script messaging to better describe performance/cycle-accuracy expectations when running Corstone FVP simulations.
Changes:
- Updates the copyright year in
run_fvp.sh. - Rewords the Corstone FVP performance/cycle-accuracy warning shown at runtime.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,5 +1,5 @@ | |||
| #!/usr/bin/env bash | |||
| # Copyright 2025 Arm Limited and/or its affiliates. | |||
| # Copyright 2026 Arm Limited and/or its affiliates. | |||
There was a problem hiding this comment.
The file previously carried a 2025 copyright header; changing it to only 2026 drops the original year. Other Arm scripts in this directory use a year range when spanning multiple years (e.g., 2025-2026). Consider using a 2025-2026 range here as well to preserve the original year and match the prevailing header pattern.
| # Copyright 2026 Arm Limited and/or its affiliates. | |
| # Copyright 2025-2026 Arm Limited and/or its affiliates. |
There was a problem hiding this comment.
God catch, @gggekov I think this needs an update.
There was a problem hiding this comment.
Yes, good fix, updated the PR
| echo "--------------------------------------------------------------------------------" | ||
| echo "Running ${elf_file} for ${target} run with FVP:${fvp_model} num_macs:${num_macs} timeout:${timeout}" | ||
| echo "WARNING: Corstone FVP is not cycle accurate and should NOT be used to determine valid runtime" | ||
| echo "The Corstone FVPs are cycle accurate for the NPU part, but note that the CPU performance is not cycle accurate." |
There was a problem hiding this comment.
This line no longer starts with a clear warning marker (e.g., "WARNING:") like other scripts (see run_vkml.sh). Keeping a consistent "WARNING:" prefix would make the performance caveat easier to spot in logs and align with existing messaging conventions.
| echo "The Corstone FVPs are cycle accurate for the NPU part, but note that the CPU performance is not cycle accurate." | |
| echo "WARNING: The Corstone FVPs are cycle accurate for the NPU part, but note that the CPU performance is not cycle accurate." |
There was a problem hiding this comment.
It's not a WARNING, it's a feature of the FVP that the NPU performance is cycle accurate. I am just telling people not to rely on the CPU performance, but the NPU performance is trustworthy.
zingo
left a comment
zingo
left a comment
There was a problem hiding this comment.
Copyright year 2025-2026, sorry for missing this in internal review.
|
Known issue Ok to merge with this fail. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gggekov
left a comment
gggekov
left a comment
There was a problem hiding this comment.
Fixed the header.
cc @digantdesai @freddan80 @per @zingo @oscarandersson8218 @mansnils @Sebastian-Larsson @robell