internal/cabi: support risc-v specific target-abi

[visualfc]

cabi support llvmTarget and targetAbi (riscv64/riscv32)

cabi arch: amd64 arm64 riscv64 arm i386 wasm riscv32 xtensa

llvmTarget

• esp32
  "llvm-target": "xtensa"

• eps32c3
  "llvm-target": "riscv32-unknown-none"
  "target-abi": "ilp32"

targetAbi

• riscv64

  • lp64
  • lp64f
  • lp64d

• riscv32

  • ilp32
  • ilp32f
  • ilp32d

[niupilot]

PR Review: RISC-V CABI Support ✅

Reviewing PR #1308 that adds RISC-V (riscv64/riscv32) and ESP32C3 target support to the CABI system.

Todo List:

• Analyze main implementation changes in internal/build/build.go
• Review CABI system changes and architecture
• Examine test data organization and consistency
• Check for potential bugs and issues
• Verify documentation consistency
• Provide comprehensive review feedback

Review Analysis

Architecture & Design

This PR significantly expands LLGO's C ABI support by adding comprehensive RISC-V target support. The implementation is well-architected with several key strengths:

✅ Strengths:

1. Modular Design: Clean separation of architecture-specific logic in TypeInfoRiscv64 and TypeInfoRiscv32 classes
2. ABI Variants: Proper support for RISC-V ABI variants (lp64, lp64f, lp64d for 64-bit; ilp32, ilp32f, ilp32d for 32-bit)
3. Target-specific Configuration: New TargetAbi field in build configuration allows fine-grained control
4. Comprehensive Test Coverage: Extensive test data across all RISC-V variants and multiple data types

Code Quality & Implementation

✅ Well-implemented:

• Type transformation logic follows established patterns from other architectures
• Proper handling of struct and array types with size-based decisions
• Float/double type handling respects ABI-specific calling conventions
• Function signature transformation maintains compatibility

⚠️ Areas for Improvement:

1. Function Signature: NewTransformer() function signature change introduces a breaking change:

// Before:
func NewTransformer(prog ssa.Program, targetName string, mode Mode, optimize bool)
// After: 
func NewTransformer(prog ssa.Program, targetName string, targetAbi string, mode Mode, optimize bool)

2. RISC-V 64-bit byval Support: Changed from return true to return false for SupportByVal(), which may affect existing functionality.

Test Data & Consistency

✅ Comprehensive Coverage:

• Test data covers all basic types (int8, int16, int32, int64, float32, float64, pointer)
• Array and struct variants properly tested
• Architecture-specific LLVM IR output looks correct

⚠️ File Organization Issues:

• Removed array_uintptr.ll files from existing architectures but added comprehensive test suites for new ones
• Some inconsistency in file naming patterns between architectures

Cross-compilation Support

✅ Good Integration:

• Proper integration with existing crosscompile system
• TargetABI field properly propagated from target configuration
• ESP32C3 correctly mapped to RISC-V32 architecture

Potential Issues

🔍 Technical Concerns:

1. Breaking Change in Public API: The NewTransformer signature change could break downstream users
2. Type Handling Complexity: RISC-V type handling logic is quite complex, especially for float types in different ABI variants
3. Memory Layout: Some type transformations may have subtle issues with memory layout assumptions

🔍 Missing Documentation:

• No updates to README.md or documentation files
• No documentation of the new RISC-V ABI support
• Missing examples of how to use the new target configurations

Specific Code Issues

// internal/cabi/arch.go:376-385
switch p.mabi {
case MABI_LP64F, MABI_LP64D:
    if types[0] == ctx.FloatType() {
        return info
    }
}

Suggestion: Consider documenting why only float32 is handled here and not float64 for LP64F.

// internal/build/build.go:216-218  
if conf.Target != "" && export.TargetABI != "" && conf.TargetAbi == "" {
    conf.TargetAbi = export.TargetABI
}

Good: Proper fallback logic for ABI configuration.

Overall Assessment

This is a high-quality implementation that adds significant value to LLGO's cross-compilation capabilities. The RISC-V support is comprehensive and follows established patterns. However, there are some concerns around breaking changes and missing documentation.

Priority Items to Address:

1. Documentation: Add documentation for RISC-V support and new ABI configuration options
2. API Stability: Consider versioning strategy for the NewTransformer function change
3. Test Organization: Review and standardize test file organization across architectures

Recommendation: ✅ Approve with suggestions - The core implementation is solid, but addressing documentation and API stability concerns would strengthen this PR significantly.

---

👉 Want me to re-run this review? Just type /review in the comments! For more usage examples, visit CodeAgent GitHub Repository.

[gemini-code-assist]

Note

The number of changes in this pull request is too large for Gemini Code Assist to generate a summary.

[niupilot]

Code Review Summary

This PR adds comprehensive RISC-V support to LLGO's C ABI system. The implementation is well-designed and follows established patterns. I've identified several areas for improvement and provided specific suggestions below.

Key Findings:

• Architecture Design: Excellent modular approach with proper ABI variant support
• Breaking Changes: API signature changes need consideration for backward compatibility
• Documentation: Missing documentation for new RISC-V features
• Test Coverage: Comprehensive test data with good coverage across all variants

Priority Improvements:

1. Add comprehensive documentation for RISC-V ABI support
2. Consider API versioning strategy for breaking changes
3. Add inline documentation for complex ABI logic
4. Standardize test file organization

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.21%. Comparing base (897f944) to head (db28648).
⚠️ Report is 26 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1308      +/-   ##
==========================================
+ Coverage   90.10%   90.21%   +0.11%     
==========================================
  Files          43       43              
  Lines       12562    12658      +96     
==========================================
+ Hits        11319    11420     +101     
+ Misses       1087     1081       -6     
- Partials      156      157       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.