Skip to content

Conversation

@no1wudi
Copy link
Contributor

@no1wudi no1wudi commented Jan 10, 2025

Summary

Added LLVM-specific configuration variables to ARM architecture CMake files:

  • LLVM_ARCHTYPE for architecture variant (thumbv6m, thumbv7a, etc)
  • LLVM_CPUTYPE for CPU target (cortex-m0, cortex-a5, etc)
  • LLVM_ABITYPE for ABI (eabi/eabihf)

Impact

New CMake flags for ARM

Testing

Tested with apache/nuttx-apps#2487

Reformatted the FPU options table to use a clearer markdown-style table format
with proper alignment and column headers. Added visual separators (~~~) to make
the table stand out from surrounding code. Improved consistency in column widths
and fixed line wrapping for better readability.

Signed-off-by: Huang Qi <[email protected]>
Added LLVM-specific configuration variables to ARM architecture CMake files:
- LLVM_ARCHTYPE for architecture variant (thumbv6m, thumbv7a, etc)
- LLVM_CPUTYPE for CPU target (cortex-m0, cortex-a5, etc)
- LLVM_ABITYPE for ABI (eabi/eabihf)

These changes enable LLVM/Clang toolchain support while maintaining
compatibility with existing GCC configurations. The LLVM variables
are set based on the same architecture/CPU/FPU configurations used
for GCC flags.

Signed-off-by: Huang Qi <[email protected]>
@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Size: S The size of the change in this PR is small labels Jan 10, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 10, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements, but is missing some key information. It provides a summary of the changes and identifies the impacted area. The testing section links to a related PR in nuttx-apps, which implies testing was performed, but lacks specific details.

Here's what's missing and needs to be addressed to fully meet the requirements:

  • Summary: Needs more detail on why these changes are necessary. What problem do they solve? What benefit do they provide? What was the previous behavior, and why was it inadequate?
  • Impact: The impact section is too brief. While it identifies new CMake flags, it doesn't explicitly answer the specific impact questions (user impact, build impact, hardware impact, documentation, security, compatibility). Even if the answer is "NO", it should be explicitly stated. For "build impact," since there are new CMake flags, it's definitely a "YES" and requires more description about how the build process changes with these flags.
  • Testing: Linking to a related PR is insufficient. The PR should contain its own testing logs demonstrating the behavior before and after the change. The linked PR may provide context, but the PR itself should be self-contained in terms of verification. Also, the build host and target details are missing.

In short, the PR provides a starting point, but needs to be fleshed out with more specific details to be considered complete.

@anchao anchao merged commit 6dbdfb3 into apache:master Jan 10, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arch: arm Issues related to ARM (32-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.

5 participants