GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Fold const bool with unary not by moreal · Pull Request #7357 · RustPython/RustPython Conversation
|
📝 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 |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| (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, | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
* Fold const bool with unary not * Fold unnecessary TO_BOOL
* Fold const bool with unary not * Fold unnecessary TO_BOOL
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
Labels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.