Zero alloc: fix bug interaction between "opt" and top-level "all" #2424
Conversation
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
cleanup after removal of closure and flambda2
- Use it in middle-end instead of backend: all decision about dropping Zero_alloc_attribute are in one place now. - Add [Location.t] argument to [Zero_alloc_attribute.from_lambda] that comes from the location of the function definition. It corresponds to the previous use of [fun_dbg] in the backend. This location is only used for reporting friendlier errors to the user when the check enabled by [@@@zero_alloc all] fails on the function. - Update call sites of [from_lambda] in the middle-end.
gretay-js
force-pushed
the
zero_alloc_all_opt2
branch
from
9b57582 to
9877b46
Compare
xclerc
reviewed
Apr 10, 2024
yes, |
|
Indeed, my expectations came from the |
xclerc
approved these changes
Apr 10, 2024
gretay-js
added a commit
to gretay-js/flambda-backend
that referenced
this pull request
Apr 11, 2024
…aml-flambda#2424) * Remove unused function from Cmm_helpers cleanup after removal of closure and flambda2 * Use [Clflags.zero_alloc_check_assert_all] earlier - Use it in middle-end instead of backend: all decision about dropping Zero_alloc_attribute are in one place now. - Add [Location.t] argument to [Zero_alloc_attribute.from_lambda] that comes from the location of the function definition. It corresponds to the previous use of [fun_dbg] in the backend. This location is only used for reporting friendlier errors to the user when the check enabled by [@@@zero_alloc all] fails on the function. - Update call sites of [from_lambda] in the middle-end. * Remove [Ignore_assert_all] from middle-end * Remove [Ignore_assert_all] from the backend * Add a new test * Fix corner case of [@@@zero_alloc all] for "opt" flag * Update test output and remove CR-soon. * Format
Forestryks
pushed a commit
to Forestryks/flambda-backend
that referenced
this pull request
Apr 17, 2024
…aml-flambda#2424) * Remove unused function from Cmm_helpers cleanup after removal of closure and flambda2 * Use [Clflags.zero_alloc_check_assert_all] earlier - Use it in middle-end instead of backend: all decision about dropping Zero_alloc_attribute are in one place now. - Add [Location.t] argument to [Zero_alloc_attribute.from_lambda] that comes from the location of the function definition. It corresponds to the previous use of [fun_dbg] in the backend. This location is only used for reporting friendlier errors to the user when the check enabled by [@@@zero_alloc all] fails on the function. - Update call sites of [from_lambda] in the middle-end. * Remove [Ignore_assert_all] from middle-end * Remove [Ignore_assert_all] from the backend * Add a new test * Fix corner case of [@@@zero_alloc all] for "opt" flag * Update test output and remove CR-soon. * Format
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The following example fails the check:
but it is expected to pass the check by default (and fail with
-check-zero-alloc allcompiler flag).This PR fixes the bug and adds a test to show the expected behavior.
The bug is that
Clflags.zero_alloc_check_assert_allis checked too late. The cause of the bug is that "opt" annotations are removed by the middle-end (from_lambdatransformation) ifLambda.is_check_enabled ~optis false, but then in the backend, ifClflags.zero_alloc_check_assert_allis true, we add "assert zero_alloc" annotations on the same function, because it doesn't have "ignore" or "assume". The fix is to check the two flags in one place in the middle-end duringfrom_lambdaconversion. The reason that we don't check it all in the backend is the interaction with the dead code elimination in flambda: it's better to remove unnecessary "asserts" earlier. We can probably move this check even earlier after #2400 is merged, but that would be a separate PR for refactoring only.As a result of the fix, we can eliminate the now-unused
Ignore_allconstructor from the middle-end andCmm.On top of #2416.Best review commit-by-commit.