On Fri, 16 Jan 2026 15:19:07 -0800 [email protected] wrote: > From: Scott Mitchell <[email protected]> > > Modify RTE_PTR_ADD and RTE_PTR_SUB to use char* pointer arithmetic > on Clang instead of uintptr_t casts. Benefits of this approach: > - API compatibility: works for both integer and pointer inputs > - Retains simple macros: no pragmas, no _Generic > - Enables Clang optimizations: Clang can now unroll and vectorize > pointer loops. GCC uses uintptr_t to avoid false positive warnings. > > Example use case which benefits is __rte_raw_cksum. Performance > results from cksum_perf_autotest on Intel Xeon (Cascade Lake, > AVX-512) built with Clang 18.1 (TSC cycles/byte): > Block size Before After Improvement > 100 0.40 0.24 ~40% > 1500 0.50 0.06 ~8x > 9000 0.49 0.06 ~8x > > Signed-off-by: Scott Mitchell <[email protected]> > > ---
Looks good to me and not too complex. AI code review had some additional comments. ## Patch Review: eal: RTE_PTR_ADD/SUB char* for compiler optimizations (v8) ### ✅ PASS Items **Commit Message - Subject Line:** - Length: ~52 characters (✓ under 60 limit) - Prefix: `eal:` is correct for EAL changes - No trailing period - Imperative mood **Commit Message - Body:** - Well-documented rationale with clear performance data - Does NOT start with "It" ✓ - Performance table shows concrete improvements (~40% to ~8x) - Line wrapping appears within 75-character limit **Tags:** - `Signed-off-by:` present with real name and valid email ✓ - Tag order is correct **SPDX/License (new file `test_ptr_add_sub.c`):** - SPDX identifier on first line ✓ - Copyright follows SPDX ✓ - Blank line after copyright ✓ - BSD-3-Clause appropriate for `app/test` ✓ **Tests:** - Comprehensive test coverage for integer and pointer types ✓ - Uses `TEST_ASSERT_EQUAL` macros ✓ - Uses `REGISTER_FAST_TEST` infrastructure ✓ - Tests both ADD and SUB with round-trip verification ✓ - Named constants (`TEST_INITVAL`, `TEST_INCREMENT`) instead of magic numbers ✓ **Code Quality:** - Macro implementation is well-documented with explanatory comment - Conditional compilation (`#ifdef RTE_CC_CLANG`) is clean - No forbidden terminology --- ### ⚠️ WARNING Items **1. Include Order (test_ptr_add_sub.c lines 161-164):** ```c #include "test.h" #include <stdint.h> #include <rte_common.h> ``` Per AGENTS.md, order should be: (1) system includes, (2) EAL includes, (3) misc DPDK includes, (4) application-specific. Should be: ```c #include <stdint.h> #include <rte_common.h> #include "test.h" ``` **2. Subject Line Contains Underscores:** The subject `eal: RTE_PTR_ADD/SUB char* for compiler optimizations` includes underscores (code identifiers after the colon). Per AGENTS.md: "Underscores after the colon (indicates code in subject)" is flagged. Consider: ``` eal: use char pointer arithmetic for Clang optimizations ``` **3. Missing Release Notes:** This is a notable performance optimization (~8x improvement in some cases). Per AGENTS.md, "significant changes" should update release notes in `doc/guides/rel_notes/`. Consider adding a note about the checksum performance improvement. **4. Doxygen Documentation:** The existing Doxygen comments for `RTE_PTR_ADD` and `RTE_PTR_SUB` remain minimal: ```c /** * add a byte-value offset to a pointer */ ``` Consider adding `@param` and `@return` documentation, especially noting that `ptr` can be either a pointer or integer type. --- ### ℹ️ INFO Items (Minor Suggestions) **1. Variable Declaration Style:** In `test_ptr_add_sub_integer_types()`, variables are declared at point-of-use (C99 style), which is acceptable per AGENTS.md. However, `void *result` is declared at function start while type-specific variables are inline—this mixing is fine but slightly inconsistent. **2. Test Structure:** The tests don't use `unit_test_suite_runner` infrastructure (they use direct function calls in `test_ptr_add_sub()`). While functional, AGENTS.md suggests using `unit_test_suite_runner` for "proper integration with the DPDK test framework." This is optional since `REGISTER_FAST_TEST` is used correctly. **3. Comment Placement:** The implementation comment explaining Clang vs GCC behavior is placed between the Doxygen comment and the macro. This works, but having implementation notes inside a `/* ... */` block separate from Doxygen is good practice (which this patch does correctly). --- ### Summary | Category | Status | |----------|--------| | Commit message format | ✅ Pass (minor subject line concern) | | SPDX/License | ✅ Pass | | Code style | ⚠️ Include order should be fixed | | Tests | ✅ Pass - comprehensive | | Documentation | ⚠️ Consider release notes | | Compilation | Should verify independently | **Overall Assessment:** This is a well-crafted v8 patch with solid test coverage and clear performance benefits. The main actionable items are fixing the include order and considering release notes for this optimization. The subject line concern about underscores is minor since `RTE_PTR_ADD/SUB` are canonical macro names in DPDK.
