Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

compiler alias analysis: Fix repeated argument bug #9356

Merged
merged 1 commit into from
Jan 30, 2025

Conversation

frej
Copy link
Contributor

@frej frej commented Jan 28, 2025

When the same variable occurs multiple times in the argument list to a call, that variable should become aliased.

This patch corrects the above described omission by renaming the current aa_alias_surviving_args/4 to aa_alias_surviving_phi_args/4 which we use for Phi-instructions and creating a new aa_alias_surviving_args/4 (for calls) which makes use of aa_alias_surviving_phi_args/4 but also does a second pass through the argument list to find repeated variables.

This patch is made against OTP-27.2.1 (the bug first appeared in OTP-27), the same functional change should also be pulled into master, where it doesn't apply cleanly. This branch contains the same change, but rebased to the current master.

When the same variable occurs multiple times in the argument list to a
call, that variable should become aliased.

This patch corrects the above described omission by renaming the
current aa_alias_surviving_args/4 to aa_alias_surviving_phi_args/4
which we use for Phi-instructions and creating a new
aa_alias_surviving_args/4 (for calls) which makes use of
aa_alias_surviving_phi_args/4 but also does a second pass through the
argument list to find repeated variables.
Copy link
Contributor

github-actions bot commented Jan 28, 2025

CT Test Results

    2 files    324 suites   9m 32s ⏱️
  820 tests   818 ✅ 2 💤 0 ❌
5 443 runs  5 441 ✅ 2 💤 0 ❌

Results for commit 6a703cc.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@bjorng bjorng added team:VM Assigned to OTP team VM fix labels Jan 29, 2025
@bjorng bjorng self-assigned this Jan 29, 2025
@bjorng bjorng added testing currently being tested, tag is used by OTP internal CI and removed testing currently being tested, tag is used by OTP internal CI labels Jan 29, 2025
bjorng
bjorng previously approved these changes Jan 29, 2025
Copy link
Contributor

@bjorng bjorng left a comment

Choose a reason for hiding this comment

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

Thanks! Added to our daily builds along with update primary bootstraps.

@frej
Copy link
Contributor Author

frej commented Jan 29, 2025

Of note is that, although the alias analysis was wrong, actual cases where the bug led to bad code seem very infrequent. For example, running diffable --co dssaopt with and without the fix and comparing the outputs shows some changes in the aliasing information. But comparing the generated assembly (i.e. without --co dssaopt), shows no changes.

@bjorng bjorng changed the base branch from maint-27 to maint January 30, 2025 15:20
@bjorng bjorng dismissed their stale review January 30, 2025 15:20

The base branch was changed.

@bjorng bjorng merged commit 05886b9 into erlang:maint Jan 30, 2025
24 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
fix team:VM Assigned to OTP team VM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants