Conversation
Added stricter Result matching in StoreLoad* and StoreLoadParam* Match methods to improve transformation accuracy. Updated IfThenElse32Same and IfThenElse64Same to use the correct operand in Move instructions. Minor comment formatting fix in IfThenElse32.
Replaced branch instructions with NOP and added calls to Framework.Transform.UpdatePhiBlock in BranchManagedPointerOnlyOneExit.cs and BranchObjectOnlyOneExit.cs to ensure Phi blocks are updated after branch removal.
Changed target assignment in Transform methods of Branch32OnlyOneExit, Branch64OnlyOneExit, BranchManagedPointerOnlyOneExit, and BranchObjectOnlyOneExit to use context.Block.NextBlocks[0] instead of context.BranchTarget1. This ensures the target is consistently selected from the block's next blocks list. No other logic was modified.
Add a check to ensure divisors do not exceed 32 bits, as GetMagicNumber only supports 32-bit values. Also, explicitly cast 64-bit constants to uint in Transform for type safety.
Changed Transform methods in LoadParamSignExtend32x64Double.cs and LoadParamZeroExtend16x64Double.cs to use IR.Move64 instead of IR.Move32, ensuring correct 64-bit data handling for these operations.
Improved boolean property setters in Node to properly clear flags when set to false, ensuring correct behavior. Updated XML documentation in Context.cs for BranchTargetsCount, and fixed a class name comment in SubCarryOut64.cs.
Correct InsertAfter() XML comment to reflect its behavior, and update the assertion to prevent insertion after block end instructions. Also, reformat copyright header.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates several MOSA compiler optimization transforms and core IR helpers to fix incorrect instruction selection, tighten transform matching criteria, and correct option/utility behavior in the framework.
Changes:
- Fix incorrect IR move widths and operand selection in a few simplification/useless transforms (e.g., 64-bit moves, IfThenElse same-operand simplification).
- Harden strength-reduction and store/load elimination transforms with additional constant/range checks and operand consistency checks.
- Fix framework utilities: properly clear
InstructionOptionflags via boolean setters, and correctContext.InsertAfter()doc/assert behavior.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/Useless/LoadParamZeroExtend16x64Double.cs | Use Move64 when replacing redundant 64-bit zero-extend. |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/Useless/LoadParamSignExtend32x64Double.cs | Use Move64 when replacing redundant 64-bit sign-extend. |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/StrengthReduction/DivUnsignedMagicNumber64.cs | Reject divisors > 32-bit and cast safely for GetMagicNumber. |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/Simplification/IfThenElse64Same.cs | Fix simplification to move the value operand (not the condition). |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/Simplification/IfThenElse32Same.cs | Fix simplification to move the value operand (not the condition). |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/Simplification/BranchObjectOnlyOneExit.cs | Remove redundant branch; now also calls UpdatePhiBlock (see review comments). |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/Simplification/BranchManagedPointerOnlyOneExit.cs | Remove redundant branch; now also calls UpdatePhiBlock (see review comments). |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/Simplification/Branch64OnlyOneExit.cs | Adjust target selection for phi update / redundant-branch removal (see review comments). |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/Simplification/Branch32OnlyOneExit.cs | Adjust target selection for phi update / redundant-branch removal (see review comments). |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/Memory/StoreLoadR8.cs | Add check ensuring stored value matches the prior load’s result. |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/Memory/StoreLoadR4.cs | Add check ensuring stored value matches the prior load’s result. |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/Memory/StoreLoadParamR8.cs | Add check ensuring stored value matches the prior load’s result. |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/Memory/StoreLoadParamR4.cs | Add check ensuring stored value matches the prior load’s result. |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/Memory/StoreLoadParamObject.cs | Add check ensuring stored value matches the prior load’s result. |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/Memory/StoreLoadParam64.cs | Add check ensuring stored value matches the prior load’s result. |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/Memory/StoreLoadParam32.cs | Add check ensuring stored value matches the prior load’s result. |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/Memory/StoreLoadObject.cs | Add check ensuring stored value matches the prior load’s result. |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/Memory/StoreLoad64.cs | Add check ensuring stored value matches the prior load’s result. |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/Memory/StoreLoad32.cs | Add check ensuring stored value matches the prior load’s result. |
| Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/ConstantFolding/SubCarryOut64.cs | Fix summary comment name mismatch. |
| Source/Mosa.Compiler.Framework/Node.cs | Make boolean option setters idempotent (set and clear flags). |
| Source/Mosa.Compiler.Framework/Context.cs | Fix file header, XML comment duplication, and InsertAfter() doc/assert correctness. |
Comments suppressed due to low confidence (2)
Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/Simplification/Branch64OnlyOneExit.cs:26
- This transform calls
UpdatePhiBlock(target)unconditionally aftercontext.SetNop(). When the block still has an unconditional jump to the sametarget, removing the conditional branch does not remove a predecessor edge, andPhiHelper.UpdatePhimay hit its debug assertion expectingOperandCount != PreviousBlocks.Count. Consider only updating phi nodes if the edge totargetwas actually removed (checkcontext.Block.NextBlocksafterSetNop()).
var target = context.Block.NextBlocks[0];
context.SetNop();
Framework.Transform.UpdatePhiBlock(target);
}
Source/Mosa.Compiler.Framework/Transforms/Optimizations/Manual/Simplification/Branch32OnlyOneExit.cs:26
- This transform calls
UpdatePhiBlock(target)unconditionally aftercontext.SetNop(). If the conditional branch is redundant because an unconditional jump still targets the same block, the predecessor set oftargetdoes not change andPhiHelper.UpdatePhican fail its debug assertion expecting a mismatch. Consider guarding the phi update so it only runs when the edge totargetwas actually removed (e.g., testcontext.Block.NextBlocks.Contains(target)afterSetNop()).
var target = context.Block.NextBlocks[0];
context.SetNop();
Framework.Transform.UpdatePhiBlock(target);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...Compiler.Framework/Transforms/Optimizations/Manual/Simplification/BranchObjectOnlyOneExit.cs
Show resolved
Hide resolved
....Framework/Transforms/Optimizations/Manual/Simplification/BranchManagedPointerOnlyOneExit.cs
Show resolved
Hide resolved
Correct bit manipulation in Neg64, RemUnsigned, ShiftLeft64, and sign/zero extension methods. Now properly handle full 64-bit values, division by zero checks, and correct bit widths for sign and zero extension. Improves accuracy and consistency in bit-level value propagation.
Refactored UpdateInstruction to more accurately determine constant values for virtual registers. Now uses MinValue when the value is a known constant, BitsSet when all bits are known, and returns early otherwise. This improves constant propagation and bit knowledge handling.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Update bit-tracking logic to use BitsSet for 64-bit zero checks instead of BitsSet32, ensuring correct handling of full 64-bit values. Also, use AreLower32BitsKnown for 32-bit ops in MulUnsigned32. Improves accuracy for multiply, divide, remainder, and shift operations.
Updated RemUnsigned32 to use value2.BitsSet32 instead of value2.BitsSet when narrowing results. This ensures correct handling of 32-bit unsigned remainder operations by applying the appropriate bit width in NarrowMax and NarrowClearBits.
Added handlers for signed division and remainder (DivSigned32, DivSigned64, RemSigned32, RemSigned64) and subtraction operations (Sub32, Sub64, SubCarryIn32, SubCarryIn64) in BitTrackerStage. Registered these in Initialize. Removed carry-out and overflow-out instruction tracking.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Reorganize and comprehensively register all relevant IR instructions in BitTrackerStage.Initialize, ensuring systematic grouping and coverage of arithmetic, logic, shift, move, load, and conversion operations. Add Result2NarrowToBoolean handler for carry/overflow outputs and introduce its implementation. Remove redundant and commented-out code for clarity and maintainability.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
1 participant
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.
No description provided.