Skip to content

Zero alloc fix flambda2 invalid no return take2#2810

Merged
gretay-js merged 7 commits intooxcaml:mainfrom
gretay-js:zero_alloc_fix_flambda2_invalid_no_return_take2
Jul 19, 2024
Merged

Zero alloc fix flambda2 invalid no return take2#2810
gretay-js merged 7 commits intooxcaml:mainfrom
gretay-js:zero_alloc_fix_flambda2_invalid_no_return_take2

Conversation

@gretay-js
Copy link
Copy Markdown
Contributor

@gretay-js gretay-js commented Jul 17, 2024

Fix compilation failure caused by the special treatment of caml_flambda2_invalid for zero_alloc (added in #2112) in the presence of unboxed types.

The middle end generates caml_flambda2_invalid with return=false and return type set to typ_void. In Selectgen, we sometimes change it to return=true and typ_int, but the context may not be compatible with the return type of int. This should not matter because the code is unreachable, but it triggers an assertion:

>> Fatal error: Illegal move between registers of differing types (I/77[%rax] to F/78[%xmm0])

The proposed fix updates return=false at the end of Zero_alloc_check on all these special primitives.

  • For Cfg, we rerun dead code elimination pass if needed. This is expensive but performed only when needed.
  • For Mach, we rely on Deadcode pass to eliminate the illegal moves before register allocation. Changing returns field is expensive because we reallocate all functions annotated with zero_alloc. A cheaper alternative (see reverted commit) is to directly change Deadcode pass.

A proper fix would be to let the middle end eliminate dead functions even if they are annotated with zero_alloc assertions (i.e., revert #1378), and instead propagate zero_alloc assertions to instructions, similarly to the way we do for assume using debug info, but currently debug info may not be reliable enough.

@gretay-js gretay-js added bug Something isn't working backend zero alloc zero-alloc check labels Jul 17, 2024
@gretay-js gretay-js requested review from TheNumbat and xclerc July 17, 2024 16:53
@TheNumbat
Copy link
Copy Markdown
Member

This approach seems fine (for now), but do we know how often the extra DCE pass will be needed? I wouldn't be surprised if caml_flambda2_invalid is generated pretty frequently.

@gretay-js
Copy link
Copy Markdown
Contributor Author

do we know how often the extra DCE pass will be needed? I wouldn't be surprised if caml_flambda2_invalid is generated pretty frequently.

it's only triggered for function explicitly annotated with zero_alloc . I expect to be seeing it more as more annotations are added, especially on interfaces, which forces annotations on many functions that aren't directly annotated by the users.

Copy link
Copy Markdown
Contributor

@xclerc xclerc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As briefly discussed off-line, we are mildly
worried of the performance implications
of the mach version, but it should be
removed soon-ish.

@gretay-js
Copy link
Copy Markdown
Contributor Author

As briefly discussed off-line, we are mildly worried of the performance implications of the mach version, but it should be removed soon-ish.

Once we have #2813, we can use this information to remove functions that are kept alive only for zero_alloc check instead of modifying them here. I expect that this condition will cover most of hte cases (not all) and then we can avoid reallocating if a function does not contain a call to caml_flambda2_invalid. I'm leaving these optimizations out of this bug-fix PR.

@gretay-js gretay-js merged commit ff6556c into oxcaml:main Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend bug Something isn't working zero alloc zero-alloc check

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants