-
Notifications
You must be signed in to change notification settings - Fork 1.5k
winch: Crank the ratchet on tests that can fail #10829
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
winch: Crank the ratchet on tests that can fail #10829
Conversation
Subscribe to Label Action
This issue or pull request has been labeled: "winch"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
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.
Glad that we can avoid ignoring tests. Thanks for this! Left one small comment inline.
This commit is the first step toward simplifying constant handling, particularly for the aarch64 backend. The main highlights in this patch are: * Introduction of `ConstantPool` implemenetation on top of Cranlift primitives * Usage of the constant pool from aarch64, which simplifies the loading of constants, particularly floating point constants. The main motivation behind this change is to _eventually_ detach the implicit usage of the scatch register from constant loading as much as possible, reducing the possibility of subtle bugs (like the one described in bytecodealliance#10829). Note that this I have a work-in-progress branch from where all these changes are cherry picked from to make everything easier to review.
This commit is the first step toward simplifying constant handling, particularly for the aarch64 backend. The main highlights in this patch are: * Introduction of `ConstantPool` implemenetation on top of Cranlift primitives. The implemettaion is identical to the existing for x64, however, it's abstracted so that it can be easily consumed from any existing backend. * Usage of the constant pool from aarch64, which simplifies the loading of constants, particularly floating point constants. The main motivation behind this change is to _eventually_ detach the implicit usage of the scatch register from constant loading as much as possible, reducing the possibility of subtle bugs (like the one described in bytecodealliance#10829). Note that I have a work-in-progress branch from where all these changes are cherry picked from, to make everything easier to review. A side effect of this change, is the improvement to the code generation involving floating point constants. Prior to this change, multiple moves were involved, with this patch, at most 1 move is required and at worst one load is required.
This commit is the first step toward simplifying constant handling, particularly for the aarch64 backend. The main highlights in this patch are: * Introduction of `ConstantPool` implemenetation on top of Cranlift primitives. The implemettaion is identical to the existing for x64, however, it's abstracted so that it can be easily consumed from any existing backend. * Usage of the constant pool from aarch64, which simplifies the loading of constants, particularly floating point constants. The main motivation behind this change is to _eventually_ detach the implicit usage of the scatch register from constant loading as much as possible, reducing the possibility of subtle bugs (like the one described in bytecodealliance#10829). Note that I have a work-in-progress branch from where all these changes are cherry picked from, to make everything easier to review. A side effect of this change, is the improvement to the code generation involving floating point constants. Prior to this change, multiple moves were involved, with this patch, at most 1 move is required and at worst one load is required.
This commit is the first step toward simplifying constant handling, particularly for the aarch64 backend. The main highlights in this patch are: * Introduction of `ConstantPool` implemenetation on top of Cranlift primitives. The implemettaion is identical to the existing for x64, however, it's abstracted so that it can be easily consumed from any existing backend. * Usage of the constant pool from aarch64, which simplifies the loading of constants, particularly floating point constants. The main motivation behind this change is to _eventually_ detach the implicit usage of the scatch register from constant loading as much as possible, reducing the possibility of subtle bugs (like the one described in bytecodealliance#10829). Note that I have a work-in-progress branch from where all these changes are cherry picked from, to make everything easier to review. A side effect of this change, is the improvement to the code generation involving floating point constants. Prior to this change, multiple moves were involved, with this patch, at most 1 move is required and at worst one load is required.
* winch(aarch64): implify constant handling, part 1/N This commit is the first step toward simplifying constant handling, particularly for the aarch64 backend. The main highlights in this patch are: * Introduction of `ConstantPool` implemenetation on top of Cranlift primitives. The implemettaion is identical to the existing for x64, however, it's abstracted so that it can be easily consumed from any existing backend. * Usage of the constant pool from aarch64, which simplifies the loading of constants, particularly floating point constants. The main motivation behind this change is to _eventually_ detach the implicit usage of the scatch register from constant loading as much as possible, reducing the possibility of subtle bugs (like the one described in #10829). Note that I have a work-in-progress branch from where all these changes are cherry picked from, to make everything easier to review. A side effect of this change, is the improvement to the code generation involving floating point constants. Prior to this change, multiple moves were involved, with this patch, at most 1 move is required and at worst one load is required. * Update disassembly tests * Apply refactored constant handling on top of shared float min/max implementation * `fmt`
2aeb9f3
to
75a4504
Compare
This commit moves all "ignore these outright" tests for Winch and AArch64 to "run the test and expect failure" after recent PRs which removed most of the segfaults/etc which prevented running these tests. One failure remained which was easy enough to fix here: the `cmov` implementation only worked on integer registers, not floating-point registers, leading to a panic. A helper was added for floating-point selects to resolve this panic and get some tests passing. After this fix the `ignore` method is no longer necessary as all tests can be run and be flagged as either pass or fail.
75a4504
to
3160410
Compare
Ok now a much smaller PR with all the previous fixes, thanks @saulecabrera! Now this just has a change for |
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.
Thanks!
I'm currently looking into it. |
Reproduced that failure locally and it required a new |
This commit moves all "ignore these outright" tests for Winch and
AArch64 to "run the test and expect failure" after recent PRs which
removed most of the segfaults/etc which prevented running these tests.
One failure remained which was easy enough to fix here: the
cmov
implementation only worked on integer registers, not floating-point
registers, leading to a panic. A helper was added for floating-point selects to
resolve this panic and get some tests passing.
After this fix the
ignore
method is no longer necessary as all testscan be run and be flagged as either pass or fail.