Use C++20 likely/unlikely instead of __builtin_expect - #1714
Conversation
|
👋 Hi iyastreb! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR replaces scattered uses of ChangesBranch Prediction Hint Modernization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/infra/nixl_memory_section.cppsrc/infra/nixl_memory_section.cpp:20:10: fatal error: 'nixl.h' file not found ... [truncated 2200 characters] ... line 54, characters 4-52 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/plugins/ucx/ucx_backend.cpp`:
- Line 158: The branch hint on the conditional checking "ret == NIXL_SUCCESS" is
inverted; change the attribute from [[unlikely]] to [[likely]] so the
successful-fast-path is optimized. Locate the if-condition that tests ret
against NIXL_SUCCESS in src/plugins/ucx/ucx_backend.cpp (the loop handling
completion of requests) and replace [[unlikely]] with [[likely]] for the if(ret
== NIXL_SUCCESS) check to match the comment and expected hot path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 581813b1-2650-454c-bdf9-976a81014d99
📒 Files selected for processing (5)
benchmark/nixlbench/src/worker/nixl/nixl_worker.cppsrc/core/nixl_agent.cppsrc/infra/nixl_memory_section.cppsrc/plugins/ucx/ucx_backend.cppsrc/plugins/ucx/ucx_enums.cpp
|
/build |
|
/build |
|
/build |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/infra/nixl_memory_section.cpp (1)
86-99: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winReview:
[[likely]]/[[unlikely]]are valid here (project uses C++20); updatedocs/CodeStyle.mdinstead.
- Root
meson.buildsetscpp_std=c++20, so[[likely]]/[[unlikely]]will compile forsrc/**/*.cppin this repo.docs/CodeStyle.mdstill states “C++17” forsrc/**/*.cpp; if the intended standard is C++20, update the guideline to match.src/infra/nixl_memory_section.cppline 184 has no[[likely]]/[[unlikely]]—it’s anormalizeSecDescassignment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/infra/nixl_memory_section.cpp` around lines 86 - 99, The code uses C++20 attributes ([[likely]]/[[unlikely]]) and Meson already sets cpp_std=c++20 in the build, so update the docs/CodeStyle.md to reflect C++20 (not C++17); edit the guideline text that mentions the language standard and any examples to state C++20 and note that attributes like [[likely]]/[[unlikely]] are allowed (no code change needed in functions using base.getCoveringIndex, s_index, query, or normalizeSecDesc).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/infra/nixl_memory_section.cpp`:
- Line 184: The diff shows a no-op change on the line calling
normalizeSecDesc(lp = normalizeSecDesc(mem, nixl_mem)) — either revert the
unintentional whitespace/formatting change so the line returns exactly to its
previous content, or if the change was intentional, replace the line with the
intended modification and add a clarifying comment above it; locate the call to
normalizeSecDesc and the variable lp (and references to mem and nixl_mem) and
either restore the original exact text or commit the real change plus a brief
explanatory comment so the PR diff matches intent.
---
Outside diff comments:
In `@src/infra/nixl_memory_section.cpp`:
- Around line 86-99: The code uses C++20 attributes ([[likely]]/[[unlikely]])
and Meson already sets cpp_std=c++20 in the build, so update the
docs/CodeStyle.md to reflect C++20 (not C++17); edit the guideline text that
mentions the language standard and any examples to state C++20 and note that
attributes like [[likely]]/[[unlikely]] are allowed (no code change needed in
functions using base.getCoveringIndex, s_index, query, or normalizeSecDesc).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3f1f328e-62b6-4ce6-8b77-bed666f2395d
📒 Files selected for processing (1)
src/infra/nixl_memory_section.cpp
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/infra/nixl_memory_section.cpp (1)
86-99: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winReview:
[[likely]]/[[unlikely]]are valid here (project uses C++20); updatedocs/CodeStyle.mdinstead.
- Root
meson.buildsetscpp_std=c++20, so[[likely]]/[[unlikely]]will compile forsrc/**/*.cppin this repo.docs/CodeStyle.mdstill states “C++17” forsrc/**/*.cpp; if the intended standard is C++20, update the guideline to match.src/infra/nixl_memory_section.cppline 184 has no[[likely]]/[[unlikely]]—it’s anormalizeSecDescassignment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/infra/nixl_memory_section.cpp` around lines 86 - 99, The code uses C++20 attributes ([[likely]]/[[unlikely]]) and Meson already sets cpp_std=c++20 in the build, so update the docs/CodeStyle.md to reflect C++20 (not C++17); edit the guideline text that mentions the language standard and any examples to state C++20 and note that attributes like [[likely]]/[[unlikely]] are allowed (no code change needed in functions using base.getCoveringIndex, s_index, query, or normalizeSecDesc).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/infra/nixl_memory_section.cpp`:
- Line 184: The diff shows a no-op change on the line calling
normalizeSecDesc(lp = normalizeSecDesc(mem, nixl_mem)) — either revert the
unintentional whitespace/formatting change so the line returns exactly to its
previous content, or if the change was intentional, replace the line with the
intended modification and add a clarifying comment above it; locate the call to
normalizeSecDesc and the variable lp (and references to mem and nixl_mem) and
either restore the original exact text or commit the real change plus a brief
explanatory comment so the PR diff matches intent.
---
Outside diff comments:
In `@src/infra/nixl_memory_section.cpp`:
- Around line 86-99: The code uses C++20 attributes ([[likely]]/[[unlikely]])
and Meson already sets cpp_std=c++20 in the build, so update the
docs/CodeStyle.md to reflect C++20 (not C++17); edit the guideline text that
mentions the language standard and any examples to state C++20 and note that
attributes like [[likely]]/[[unlikely]] are allowed (no code change needed in
functions using base.getCoveringIndex, s_index, query, or normalizeSecDesc).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3f1f328e-62b6-4ce6-8b77-bed666f2395d
📒 Files selected for processing (1)
src/infra/nixl_memory_section.cpp
🛑 Comments failed to post (1)
src/infra/nixl_memory_section.cpp (1)
184-184:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLine marked as changed but no visible modification.
Line 184 is annotated as changed (~) but contains no visible
[[likely]]/[[unlikely]]modification or__builtin_expectreplacement. This appears inconsistent with the PR objective.If this is an unintentional change (whitespace, formatting), it should be reverted to keep the diff focused. If intentional, please clarify the change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/infra/nixl_memory_section.cpp` at line 184, The diff shows a no-op change on the line calling normalizeSecDesc(lp = normalizeSecDesc(mem, nixl_mem)) — either revert the unintentional whitespace/formatting change so the line returns exactly to its previous content, or if the change was intentional, replace the line with the intended modification and add a clarifying comment above it; locate the call to normalizeSecDesc and the variable lp (and references to mem and nixl_mem) and either restore the original exact text or commit the real change plus a brief explanatory comment so the PR diff matches intent.
|
/build |
5 participants
What?
Use C++20 likely/unlikely instead of __builtin_expect
Why?
Prefer C++20 standard constructs
Summary by CodeRabbit