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.

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

Set correct movableApartFromEffects flag for datacopy() #15556

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

cameel
Copy link
Member

@cameel cameel commented Oct 30, 2024

Discovered in #15535 (comment).

The PR replaces hard-coded side-effects with a helper that ensures they're identical with those of the DATACOPY instruction, which the builtin uses internally.

The discrepancy was introduced in a refactor (#9283) and appears to have been a mistake.

Fortunately it had no user-visible effects. The only place where the flag is used is the LoopInvariantCodeMotion optimizer step, via SideEffectsCollector::movableRelativeTo():

if (
!m_sideEffects.movableApartFromEffects ||
m_sideEffects.storage == SideEffects::Write ||
m_sideEffects.otherState == SideEffects::Write ||
m_sideEffects.memory == SideEffects::Write ||
m_sideEffects.transientStorage == SideEffects::Write
)
return false;

Since m_sideEffects.memory == SideEffects::Write makes the condition true anyway, the value of this flag does not actually matter for this instruction.

- Side effects should have been identical to those of `CODECOPY`, but movableApartFromEffects was different.
Copy link

This pull request is stale because it has been open for 14 days with no activity.
It will be closed in 7 days unless the stale label is removed.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Nov 14, 2024
@cameel cameel removed the stale The issue/PR was marked as stale because it has been open for too long. label Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant