Conversation
There was a problem hiding this comment.
Pull request overview
This pull request addresses multiple critical bugs in the MOSA compiler's Sparse Conditional Constant Propagation (SCCP) optimization and related transformation code. The fixes include correcting infinite loop bugs in phi node handling, fixing type mismatches in 32-bit vs 64-bit operations, addressing logic errors in constant propagation, and improving the robustness of the SCCP algorithm. The PR also adds comprehensive regression tests for the fixed SCCP bugs involving null checks in while loops traversing linked objects.
Changes:
- Fixed critical loop variable bugs in PhiHelper that caused infinite loops by using 'index' instead of 'i' in for loop conditions
- Corrected 32-bit vs 64-bit constant and operation mismatches across multiple transformation files (PhiR4Update, PhiR8Update, StrengthReduction transforms, BitTrackerStage)
- Fixed logic errors in BaseTransform (AreSame comparisons, Contains conditions, max value checks, window validation)
- Refactored SCCP to use fixed-size arrays instead of dynamic lists for better performance, fixed null reference handling, and corrected multiple logic errors
- Added 7 comprehensive regression tests for SCCP bug fixes involving while loops with object traversal
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Source/Mosa.Utility.SourceCodeGenerator/BuildTransformations.cs | Fixed result2 parameter to use correct type conversion (R8/To64 instead of R4/To32) and changed if statements to else if for mutually exclusive constant type checks |
| Source/Mosa.UnitTests/FlowControl/WhileTests.cs | Added 7 regression tests with LinkedNode class to verify SCCP correctly handles null checks in while loops traversing object graphs |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/PHI/PhiR8Update.cs | Corrected instruction type from IR.Phi32 to IR.PhiR8 |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/PHI/PhiR4Update.cs | Corrected instruction type from IR.Phi32 to IR.PhiR4 |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/ConstantFolding/Switch.cs | Added bounds check (index >= 0), changed to use BranchTargetsCount, and improved comments |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Auto/StrengthReduction/SubOverflowOut64ByZero.cs | Changed Constant32_0 to Constant64_0 and Move32 to Move64 for 64-bit operations |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Auto/StrengthReduction/SubCarryOut64ByZero.cs | Changed Constant32_0 to Constant64_0 and Move32 to Move64 for 64-bit operations |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Auto/StrengthReduction/MulOverflowOut64ByZero.cs | Removed unused e2 variable and changed Move32 to Move64 with correct 64-bit constant |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Auto/StrengthReduction/MulOverflowOut64ByOne.cs | Changed Constant32_0 to Constant64_0 and Move32 to Move64 for 64-bit operations |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Auto/StrengthReduction/MulCarryOut64ByZero.cs | Removed unused e2 variable and changed Move32 to Move64 with correct 64-bit constant |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Auto/StrengthReduction/MulCarryOut64ByOne.cs | Changed Constant32_0 to Constant64_0 and Move32 to Move64 for 64-bit operations |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Auto/StrengthReduction/AddOverflowOut64ByZero.cs | Changed Constant32_0 to Constant64_0 and Move32 to Move64 for 64-bit operations |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Auto/StrengthReduction/AddCarryOut64ByZero.cs | Changed Constant32_0 to Constant64_0 and Move32 to Move64 for 64-bit operations in both transformations |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Auto/BitValue/AddCarryOut64ButNotSigned.cs | Changed Move32 to Move64 (but still has bug with Constant32_0) |
| Source/Mosa.Compiler.Framework/Stages/BitTrackerStage.cs | Fixed Add64 to use BitsSet instead of BitsSet32 for 64-bit addition |
| Source/Mosa.Compiler.Framework/PhiHelper.cs | Fixed infinite loop bugs by using loop variable 'i' instead of 'index' in for loop conditions (lines 28 and 56) |
| Source/Mosa.Compiler.Framework/BitValue.cs | Fixed AreAll64BitsKnown redundant check, IsZeroOrOne logic (MinValue to MaxValue), SetStable operator (&& instead of &), and improved ToString with ToBinaryString helper |
| Source/Mosa.Compiler.Framework/BaseTransform.cs | Fixed AreSame to compare operand2 properties, Contains to use OR instead of AND, removed duplicate IsResolvedConstant checks, swapped IsSignedMax32/IsUnsignedMax32 implementations, and fixed window checks (> 0 to <= 0) |
| Source/Mosa.Compiler.Framework/Analysis/SparseConditionalConstantPropagation.cs | Major refactoring: replaced List with fixed-size array for constants, changed KeyedList to HashSet for phi statements, fixed IfThenElse logic error, corrected Not64 check, fixed ZeroExtend16x32 cast, fixed Result2 reference, improved phi handling, and numerous other improvements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.../Mosa.Compiler.Framework/Transforms/Optimizations/Auto/BitValue/AddCarryOut64ButNotSigned.cs
Outdated
Show resolved
Hide resolved
|
@copilot open a new pull request to apply changes based on the comments in this thread |
No description provided.