fix simplemath build, trigger ci on benchmark changes#6722
Conversation
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6722 +/- ##
==========================================
+ Coverage 93.90% 93.97% +0.06%
==========================================
Files 933 933
Lines 310041 310139 +98
==========================================
+ Hits 291140 291440 +300
+ Misses 18901 18699 -202 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
This PR aims to fix build issues around the simplemath implementation (notably INFINITY) and to ensure CI workflows run when benchmark sources change.
Changes:
- Define
INFINITYinsimplemath.h(guarded) and remove the localINFINITYmacro fromsimplemath.cpp. - Adjust the LLM benchmark source includes.
- Update multiple GitHub Actions workflows to trigger on
benchmark/**changes (excludingbenchmark/README.md), and tweak the linux-aarch64 simplestl+simplemath CMake invocation.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/simplemath.h | Adds a guarded INFINITY macro for simplemath builds. |
| src/simplemath.cpp | Removes the local INFINITY macro definition. |
| benchmark/benchncnn_llm.cpp | Removes <math.h> include (but file uses INFINITY). |
| .github/workflows/windows.yml | Trigger CI on benchmark path changes (excluding README). |
| .github/workflows/macos.yml | Trigger CI on benchmark path changes (excluding README). |
| .github/workflows/linux-x64-sde.yml | Trigger CI on benchmark path changes (excluding README). |
| .github/workflows/linux-x64-gpu-gcc.yml | Trigger CI on benchmark path changes (excluding README). |
| .github/workflows/linux-x64-gpu-clang.yml | Trigger CI on benchmark path changes (excluding README). |
| .github/workflows/linux-x64-cpu-gcc.yml | Trigger CI on benchmark path changes (excluding README). |
| .github/workflows/linux-x64-cpu-gcc-musl.yml | Trigger CI on benchmark path changes (excluding README). |
| .github/workflows/linux-x64-cpu-clang.yml | Trigger CI on benchmark path changes (excluding README). |
| .github/workflows/linux-riscv64.yml | Trigger CI on benchmark path changes (excluding README). |
| .github/workflows/linux-aarch64.yml | Trigger CI on benchmark path changes and removes explicit benchmark-disable for simplestl+simplemath job. |
Comments suppressed due to low confidence (1)
benchmark/benchncnn_llm.cpp:8
- This file uses
INFINITY(e.g., for-INFINITYin attention_mask), but<math.h>was removed and none of the included ncnn headers defineINFINITY. This will fail to compile on toolchains that don’t predefineINFINITY. Re-add<math.h>/<cmath>or switch to a local definition (e.g.,std::numeric_limits<float>::infinity()if available).
#include <float.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
No description provided.