perf(codegen): lower proven booleans into native numeric ops - #8031
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughProven native Booleans now convert directly to ChangesBoolean numeric lowering
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: ⚪ Minimal · up to The PR makes a focused boolean numeric-lowering change with passing validation; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant BooleanExpression
participant ExprLowering
participant BinaryLowering
participant LLVM
BooleanExpression->>ExprLowering: provide proven i1 value
ExprLowering->>LLVM: emit uitofp i1 to f64
BinaryLowering->>LLVM: emit native arithmetic or ordered fcmp
BooleanExpression->>BinaryLowering: provide invalidated or declared-only value
BinaryLowering->>LLVM: retain dynamic coercion and dispatch
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
proggeramlug
left a comment
There was a problem hiding this comment.
Blocking on exact head 7a1f654. I reproduced a semantic regression with a focused IR test: let flag = true; let liar: number = "4" as any; flag < (liar + 1). JavaScript evaluates this as true because the RHS is "41". This head emits a raw fadd on the NaN-boxed string followed by fcmp olt, producing false, and omits js_rel_lt. The new relational gate accepts the nested Add because expr_produces_canonical_raw_f64 blanket-admits Binary via type-level is_numeric_expr, even though a declared-only Add tree can carry a boxed/string result. Please make the Boolean gate reject declared-only/noncanonical nested binary trees and add this regression. This is independent proof from emitted IR, not a CI result. No version bump needed.
7a1f654 to
490958a
Compare
|
Addressed in 490958a. The fix makes the existing proof boundary explicit in both places involved:
I added the exact Validated with all 968 perry-codegen library tests, the affected-crate sweep, and the optimized |
proggeramlug
left a comment
There was a problem hiding this comment.
Re-audited exact head 490958a against the previously demonstrated nested declared-only numeric regression. The corrected IR retains both js_dynamic_string_or_number_add and js_rel_lt for a Number-annotated local holding a string, while the positive proven-boolean case retains native fcmp. The new declared-only seed and canonical-f64 rejection are aligned, the focused boolean-number tests pass, and the full perry-codegen library suite passes (968/968). No blocking issue found.
Summary
i1literals and locals directly tof64withuitofpfor arithmetic consumersfcmpinstead ofjs_rel_*as anysemanticsThis is a focused Lever E slice that advances #5497; it does not close the broader tracker.
Validation
cargo test -p perry-codegen boolean_number_tests --libcargo test -p perry-codegen --lib(967 passed)./scripts/test_affected_crates.sh --base origin/maintest_bool_arithmetic(1/1 passed)python3 scripts/check_test_registration.py./scripts/pre-tag-check.sh --quick(all relevant checks passed; the run reports the existing GC inventory ratchet at unchangedcrates/perry-codegen/src/expr/property_set.rs:1457)No version files were changed. The required PR-numbered changelog fragment is included.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation