Fold const bool with unary not#7357
Merged
youknowone merged 2 commits intoRustPython:mainfrom Mar 5, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request adds test scaffolding with optimization-specific helpers and introduces new constant-folding optimization cases for boolean expressions in the IR peephole optimizer. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Adds targeted peephole optimizations to RustPython’s codegen pipeline to fold boolean-constant not patterns and remove redundant boolean conversions in optimized builds.
Changes:
- Extend
CodeInfo::peephole_optimizeto (1) removeTO_BOOLafterLOAD_CONSTfor boolean constants and (2) foldLOAD_CONST <bool> + UNARY_NOTinto a singleLOAD_CONST <inverted bool>. - Add an optimized-compilation test helper and a new snapshot test to validate the optimized bytecode for
x = not True.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
crates/codegen/src/ir.rs |
Implements the peephole folding/removal rules for boolean-constant patterns. |
crates/codegen/src/compile.rs |
Adds compile_exec_optimized and a snapshot test that exercises the new optimization under optimize=1. |
crates/codegen/src/snapshots/rustpython_codegen__compile__tests__const_bool_not_op.snap |
New snapshot asserting the expected optimized disassembly output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/codegen/src/compile.rs (1)
9152-9159: Add the complementarynot Falsefold test to lock both constant branches.Line 9154 currently validates only one side of the bool-negation fold. Adding the
Falsecase will better guard regressions in the new peephole rule.Suggested test addition
#[test] fn test_const_bool_not_op() { assert_dis_snapshot!(compile_exec_optimized( "\ x = not True " )); } + +#[test] +fn test_const_bool_not_op_false() { + assert_dis_snapshot!(compile_exec_optimized( + "\ +x = not False +" + )); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/codegen/src/compile.rs` around lines 9152 - 9159, Add a new unit test to complement test_const_bool_not_op that asserts the optimizer folds `not False` as well as `not True`; specifically create a test (e.g., test_const_bool_not_op_false) that calls assert_dis_snapshot!(compile_exec_optimized("x = not False\n")) so both constant branches of the bool-negation peephole are covered and locked against regressions, mirroring the existing test that uses compile_exec_optimized and assert_dis_snapshot!.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/codegen/src/ir.rs`:
- Around line 705-722: The peephole for (Instruction::LoadConst { consti },
Instruction::UnaryNot) unconditionally inserts a new Boolean constant with the
negated value which can orphan the original co_consts entry; instead, before
calling self.metadata.consts.insert_full, search the constant pool for an
existing ConstantData::Boolean { value: !value } and reuse its index (const_idx)
if found, and only call insert_full when no matching constant exists; update the
returned tuple to use that const_idx so you never create a duplicate orphan
constant (referencing Instruction::LoadConst, Instruction::UnaryNot,
ConstantData::Boolean, self.metadata.consts.insert_full and
remove_unused_consts()/peephole to locate the logic).
---
Nitpick comments:
In `@crates/codegen/src/compile.rs`:
- Around line 9152-9159: Add a new unit test to complement
test_const_bool_not_op that asserts the optimizer folds `not False` as well as
`not True`; specifically create a test (e.g., test_const_bool_not_op_false) that
calls assert_dis_snapshot!(compile_exec_optimized("x = not False\n")) so both
constant branches of the bool-negation peephole are covered and locked against
regressions, mirroring the existing test that uses compile_exec_optimized and
assert_dis_snapshot!.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 42e952a2-d286-4c6e-82ec-7120113ced65
⛔ Files ignored due to path filters (1)
crates/codegen/src/snapshots/rustpython_codegen__compile__tests__const_bool_not_op.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
crates/codegen/src/compile.rscrates/codegen/src/ir.rs
Comment on lines
+705
to
+722
| (Instruction::LoadConst { consti }, Instruction::UnaryNot) => { | ||
| let constant = &self.metadata.consts[consti.get(curr.arg) as usize]; | ||
| match constant { | ||
| ConstantData::Boolean { value } => { | ||
| let (const_idx, _) = self | ||
| .metadata | ||
| .consts | ||
| .insert_full(ConstantData::Boolean { value: !value }); | ||
| Some(( | ||
| (Instruction::LoadConst { | ||
| consti: Arg::marker(), | ||
| }), | ||
| OpArg::new(const_idx as u32), | ||
| )) | ||
| } | ||
| _ => None, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Const-fold inserts can leave dead co_consts entries.
This branch adds a new boolean constant but can orphan the original one. Since remove_unused_consts() runs before peephole, unused constants introduced here are not pruned.
💡 Suggested fix
diff --git a/crates/codegen/src/ir.rs b/crates/codegen/src/ir.rs
@@
if opts.optimize > 0 {
self.dce();
self.peephole_optimize();
+ self.remove_unused_consts();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/codegen/src/ir.rs` around lines 705 - 722, The peephole for
(Instruction::LoadConst { consti }, Instruction::UnaryNot) unconditionally
inserts a new Boolean constant with the negated value which can orphan the
original co_consts entry; instead, before calling
self.metadata.consts.insert_full, search the constant pool for an existing
ConstantData::Boolean { value: !value } and reuse its index (const_idx) if
found, and only call insert_full when no matching constant exists; update the
returned tuple to use that const_idx so you never create a duplicate orphan
constant (referencing Instruction::LoadConst, Instruction::UnaryNot,
ConstantData::Boolean, self.metadata.consts.insert_full and
remove_unused_consts()/peephole to locate the logic).
youknowone
approved these changes
Mar 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request includes two optimizations:
LOAD_CONST True(orFalse) andUNARY_NOTinto one.TO_BOOLwhen it followsLOAD_CONST True, as it's redundant.While the former might not be a common scenario, I believe the latter will be quite useful.
For
x = not True, previously, it was compiled into the following lines:After this pull request, it will be compiled like the following lines:
Summary by CodeRabbit
Optimization
Tests