Skip to content
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

arch/risc-v: inline up_cpu_index if RISCV_PERCPU_SCRATCH is not enabled #15381

Merged
merged 1 commit into from
Dec 31, 2024

Conversation

no1wudi
Copy link
Contributor

@no1wudi no1wudi commented Dec 30, 2024

Summary

Inline the short function to provide better performance.

Impact

RISCV Only

Testing

Local machine & CI

@github-actions github-actions bot added Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Size: S The size of the change in this PR is small labels Dec 30, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 30, 2024

[Experimental Bot, please feedback here]

No, this PR description does not adequately meet the NuttX requirements. While concise, it lacks crucial details. Here's a breakdown:

Missing Information in Summary:

  • Why is inlining necessary? Quantify the performance improvement expected. "Better performance" is vague. Provide benchmarks or profiling data if possible.
  • What function is being inlined? The PR needs to specify the exact function name.
  • How does the inlining work? This seems trivial for inlining, but a brief mention of the mechanism (e.g., compiler optimization, manual inlining) would be helpful.
  • Issue references: If this addresses a reported performance issue, link the relevant NuttX issue.

Missing Information in Impact:

  • Is new feature added? Is existing feature changed? This is a performance optimization, so it's likely changing an existing feature. State that explicitly.
  • Impact on user: Even if the answer is NO, explicitly state it.
  • Impact on build: Likely NO, but state it explicitly.
  • Impact on hardware: While it says "RISCV Only," it needs to specify which RISC-V platforms and why only RISC-V. Is this a change to RISC-V-specific code? Does the inlining optimization only apply to RISC-V compilers? Be specific.
  • Impact on documentation: Likely NO, but state it explicitly.
  • Impact on security, compatibility: Explicitly state NO for each.

Missing Information in Testing:

  • Build Host(s): Be specific about the OS, CPU, and compiler version used for local testing.
  • Target(s): Be very specific about the RISC-V architecture, board, and configuration tested. "sim" is not enough detail if a simulator was used.
  • Testing logs: Providing "before" and "after" logs is essential. These logs should demonstrate the performance improvement. Simply saying "Local machine & CI" is insufficient. Include relevant performance metrics (e.g., execution time, CPU cycles). CI results can be linked later when available, but initial local testing results are crucial for review.

Example of an Improved PR Description:

## Summary

This PR inlines the `function_name()` function to improve performance on RISC-V platforms. Profiling revealed that `function_name()` is a frequent call site and inlining it is expected to reduce function call overhead by approximately X%. This addresses the performance issue reported in [NuttX Issue #XYZ].

## Impact

* **Is new feature added? Is existing feature changed?**  YES, existing feature (performance of `function_name()`) is changed.
* **Impact on user:** NO
* **Impact on build:** NO
* **Impact on hardware:** YES, this change specifically targets RISC-V platforms (e.g., `board1:config1`, `board2:config2`). The optimization is implemented using a compiler-specific attribute that is only available for the RISC-V compiler used.
* **Impact on documentation:** NO
* **Impact on security:** NO
* **Impact on compatibility:** NO
* **Anything else to consider:** N/A

## Testing

I confirm that changes are verified on local setup and works as intended:
* **Build Host(s):** Linux (Ubuntu 20.04), x86_64, GCC 10.2.1
* **Target(s):** RISC-V (QEMU virt, config: `xyz_config`), SiFive HiFive Unleashed (config: `abc_config`)

Testing logs before change:

Output of time command or other relevant performance measurement tool

...
Execution time: 1.234 seconds
...


Testing logs after change:

Output of time command or other relevant performance measurement tool

...
Execution time: 0.987 seconds
...

@no1wudi no1wudi force-pushed the index branch 2 times, most recently from 088a089 to c082ac0 Compare December 30, 2024 06:11
Inline the short function to provide better performance.

Signed-off-by: Huang Qi <[email protected]>
@acassis acassis merged commit 6eb2f33 into apache:master Dec 31, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants