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

compiler: add HAVE_EXPRESSION_STATEMENT macro for gcc #14580

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

GUIDINGLI
Copy link
Contributor

Summary

compiler: add HAVE_EXPRESSION_STATEMENT macro for gcc

this will open fast version of div_const when meet 64/32 when use gcc compiling

Impact

div_const in
include/nuttx/lib/math32.h

Testing

CI

@github-actions github-actions bot added Area: OS Components OS Components issues Size: XS The size of the change in this PR is very small labels Oct 31, 2024
@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 and removed Size: XS The size of the change in this PR is very small labels Nov 4, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 4, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements as described. While it provides a summary of the change, it lacks crucial details. Here's a breakdown:

Missing Information:

  • Summary:

    • Why is this change necessary? What problem does it solve? Is the current division slow? Are there benchmarks showing the improvement? Just stating it "opens a fast version" isn't sufficient.
    • How exactly does it work? What is HAVE_EXPRESSION_STATEMENT and how does its presence affect div_const? What code paths are changed? Provide a snippet if possible.
    • Related Issues: Are there any related NuttX or NuttX Apps issues this PR addresses?
  • Impact: The impact section is severely lacking. It only mentions the affected file. Address all the impact points:

    • New/Changed Feature? Is this an optimization of an existing feature or a new one?
    • Impact on user, build, hardware, documentation, security, compatibility: Even if the answer is "NO" for each, explicitly state it. If there is an impact, describe it. For example, the build impact is likely YES as a new macro is introduced.
  • Testing:

    • Build Host(s): Be specific. "CI" isn't enough. List the actual CI environments used (e.g., "GitHub Actions: Ubuntu Latest, macOS Latest").
    • Target(s): What architectures and boards were tested? "sim", "RISC-V", "ARM" are too broad. Specify the specific simulator, RISC-V board, and ARM board.
    • Logs: Providing "before" and "after" logs is crucial. These should demonstrate the performance improvement or the corrected behavior. Empty code blocks are unacceptable.

Example of Improved Summary & Impact:

## Summary

This PR introduces the `HAVE_EXPRESSION_STATEMENT` macro to enable a faster version of `div_const` when compiling with GCC.  Currently, the 64/32 division is inefficient [link to benchmark/profiling data if available]. This macro allows the compiler to optimize the division operation, resulting in a performance improvement [quantify the improvement if possible]. This change modifies the `div_const` function in `include/nuttx/lib/math32.h`.

## Impact

* Is new feature added? NO (optimization of existing feature)
* Is existing feature changed? YES (`div_const` performance improved)
* Impact on user: NO
* Impact on build: YES (new macro `HAVE_EXPRESSION_STATEMENT` needs to be handled by the build system)
* Impact on hardware: NO
* Impact on documentation: YES (documentation should be updated to explain the new macro and its impact on `div_const`)
* Impact on security: NO
* Impact on compatibility: NO
* Anything else to consider:  [mention anything relevant, e.g., potential impact on code size]

By providing the missing details and explicitly addressing each point in the template, the PR will be much clearer and easier to review, ultimately increasing the chances of it being accepted.

@@ -185,8 +185,10 @@

#if (MSEC_PER_TICK * USEC_PER_MSEC) == USEC_PER_TICK
# define MSEC2TICK(msec) div_const_roundup(msec, MSEC_PER_TICK)
# define MSEC2TICKSLOW(msec) (((msec)+(MSEC_PER_TICK/2))/MSEC_PER_TICK)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why need slow version

this will open fast version of div_const which defined in:
include/nuttx/lib/math32.h
when meet 64/32 when use gcc compiling

Signed-off-by: ligd <liguiding1@xiaomi.com>
In file included from /home/ligd/platform/mainline/nuttx/include/nuttx/clock.h:38,
                 from sam_automount.c:35:
sam_automount.c:92:19: error: braced-group within expression allowed only inside a function
   92 |     .ddelay     = MSEC2TICK(CONFIG_SAMA5D4EK_HSMCI0_AUTOMOUNT_DDELAY),
      |                   ^~~~~~~~~
sam_automount.c:93:19: error: braced-group within expression allowed only inside a function
   93 |     .udelay     = MSEC2TICK(CONFIG_SAMA5D4EK_HSMCI0_AUTOMOUNT_UDELAY),
      |                   ^~~~~~~~~

Signed-off-by: ligd <liguiding1@xiaomi.com>
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 Arch: xtensa Issues related to the Xtensa architecture Area: OS Components OS Components issues Board: arm Board: risc-v 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