Skip to content
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.

Sign up for GitHub

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

Jump to bottom

compiler: Do strength reduction of the hint for update_record #8726

Merged
merged 1 commit into from
Aug 20, 2024
Merged

compiler: Do strength reduction of the hint for update_record #8726

merged 1 commit into from
Aug 20, 2024

Conversation

frej
Copy link
Contributor

@frej frej commented Aug 16, 2024

Change the hint from reuse to copy when reusing clearly will not work.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Aug 16, 2024 •
edited
Loading

CT Test Results

    2 files    321 suites   12m 11s ⏱️
  817 tests   815 ✅ 2 💤 0 ❌
5 380 runs  5 378 ✅ 2 💤 0 ❌

Results for commit 86af471.

♻️ 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 approved these changes Aug 16, 2024
View reviewed changes
lib/compiler/src/beam_ssa_opt.erl Outdated Show resolved Hide resolved
@frej frej force-pushed the frej/no-reuse-strength-reduction-v2 branch from 933a986 to a6eacce Compare August 19, 2024 06:47
@frej
Copy link
Contributor Author

frej commented Aug 19, 2024

Updated PR to make inhibits_reuse/2 short-circuiting and extend the commit message to explain why this is not a module-global optimization.

@frej frej force-pushed the frej/no-reuse-strength-reduction-v2 branch from a6eacce to f0c9191 Compare August 19, 2024 06:57
@IngelaAndin IngelaAndin added the team:VM Assigned to OTP team VM label Aug 19, 2024
@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 Aug 19, 2024
@bjorng
Copy link
Contributor

bjorng commented Aug 19, 2024

There are merge conflicts because of the recent merge of your other pull request. Please rebase on the latest master and resolve the conflicts.

@bjorng @frej
compiler: Do strength reduction of the hint for update_record
86af471 
Change the hint from `reuse` to `copy` when reusing clearly
will not work.

Extending this optimization to be module-global, and not just work at
the function level, increases the time spent on this pass by a factor
of between 4 and 5 and only triggers infrequently, and for example,
never allows for additional destructive updates. For the moment this
is considered good enough.

Co-authored-by: Frej Drejhammar <frej.drejhammar@gmail.com>
@frej frej force-pushed the frej/no-reuse-strength-reduction-v2 branch from f0c9191 to 86af471 Compare August 19, 2024 11:48
@frej
Copy link
Contributor Author

frej commented Aug 19, 2024

Please rebase on the latest master and resolve the conflicts.

Done, will update #8695 when this one is merged.

@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 Aug 19, 2024
@bjorng
Copy link
Contributor

bjorng commented Aug 19, 2024

Thanks! Added to our daily builds.

@bjorng bjorng merged commit 141120a into erlang:master Aug 20, 2024
16 checks passed
@frej frej deleted the frej/no-reuse-strength-reduction-v2 branch August 20, 2024 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorng bjorng bjorng approved these changes

@jhogberg jhogberg jhogberg left review comments

Assignees

@bjorng bjorng

Labels
team:VM Assigned to OTP team VM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@frej @bjorng @jhogberg @IngelaAndin