Skip to content

Use deep effects in areConsecutiveInputsEqual#8808

Merged
tlively merged 2 commits into
mainfrom
consecutive-inputs-deep-left-effects
Jun 6, 2026
Merged

Use deep effects in areConsecutiveInputsEqual#8808
tlively merged 2 commits into
mainfrom
consecutive-inputs-deep-left-effects

Conversation

@tlively
Copy link
Copy Markdown
Member

@tlively tlively commented Jun 5, 2026

Although it is true that side effects in the children of the LHS input affect both the LHS and RHS inputs equally, we did not properly account for the fact that the same effects are executed again as part of the RHS and can change its value.

Although it is true that side effects in the children of the LHS input affect both the LHS and RHS inputs equally, we did not properly account for the fact that the same effects are executed again as part of the RHS and can change its value.
@tlively tlively requested a review from a team as a code owner June 5, 2026 22:17
@tlively tlively requested review from aheejin and removed request for a team June 5, 2026 22:17
Comment on lines +2885 to +2886
// Does the left-hand parent expression or side effects in the right-hand
// children affect the values flowing into the right-hand parent expression?
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This sounds a little confusing to me. What does "left-hand parent expression or side effects in the right-hand children" mean? I think what we want to ensure is LHS parent + LHS children should not affect RHS children, right?

Also even though technically LHS and RHS are the same here, it would be nice to comment that, because we just computed RHS effects above and are saying LHS effects here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This sounds a little confusing to me. What does "left-hand parent expression or side effects in the right-hand children" mean? I think what we want to ensure is LHS parent + LHS children should not affect RHS children, right?

But it's not the execution LHS children that causes the RHS to have a different value from the LHS, it's the execution of the RHS children (or possibly the LHS parent expression).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What I meant by the LHS children were the parameters for the LHS function, and LHS parent was the LHS function execution itself. Did you mean that too?

Then, wasn't it the LHS children's execution (which increased the global's value) that causes RHS's parameters (childrens) to have different values? Maybe we are talking about the same thing with a slightly different terms.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

What I meant by the LHS children were the parameters for the LHS function, and LHS parent was the LHS function execution itself. Did you mean that too

Yes 👍

Then, wasn't it the LHS children's execution (which increased the global's value) that causes RHS's parameters (childrens) to have different values? Maybe we are talking about the same thing with a slightly different terms.

No, because the LHS children execute before the LHS function call, so their side effects are already visible in the final LHS value. It's those same child expressions executing on the RHS before the RHS function call that make the RHS value different from the LHS value.

;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $function-effects-interfere (result f32)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe some comments on why this shouldn't be optimized?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done.

@tlively tlively enabled auto-merge (squash) June 6, 2026 00:54
@tlively tlively merged commit 06000bf into main Jun 6, 2026
16 checks passed
@tlively tlively deleted the consecutive-inputs-deep-left-effects branch June 6, 2026 00:57
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.

2 participants