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

Handle concrete values in CodeFolding #7117

Merged
merged 4 commits into from
Nov 27, 2024
Merged

Handle concrete values in CodeFolding #7117

merged 4 commits into from
Nov 27, 2024

Conversation

tlively
Copy link
Member

@tlively tlively commented Nov 26, 2024

CodeFolding previously only worked on blocks that did not produce
values. It worked on Ifs that produced values, but only by accident; the
logic for folding matching tails was not written to support tails
producing concrete values, but it happened to work for Ifs because
subsequent ReFinalize runs fixed all the incorrect types it produced.

Improve the power of the optimization by explicitly handling tails that
produce concrete values for both blocks and ifs. Now that the core logic
handles concrete values correctly, remove the unnecessary ReFinalize
run.

Also remove the separate optimization of Ifs with identical arms; this
optimization requires ReFinalize and is already performed by
OptimizeInstructions.

CodeFolding previously only worked on blocks that did not produce
values. It worked on Ifs that produced values, but only by accident; the
logic for folding matching tails was not written to support tails
producing concrete values, but it happened to work for Ifs because
subsequent ReFinalize runs fixed all the incorrect types it produced.

Improve the power of the optimization by explicitly handling tails that
produce concrete values for both blocks and ifs. Now that the core logic
handles concrete values correctly, remove the unnecessary ReFinalize
run.
@tlively tlively requested a review from kripken November 26, 2024 03:58
;; CHECK-NEXT: (then
;; CHECK-NEXT: )
;; CHECK-NEXT: (else
;; CHECK-NEXT: )
Copy link
Member

Choose a reason for hiding this comment

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

This output pattern of empty if-elses was better optimized before, I think - we dropped the condition and left a single copy of the body (which in this case would have been a single nop).

Copy link
Member Author

Choose a reason for hiding this comment

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

This regression is because I removed the optimization that is redundant with OptimizeInstructions. I was going to keep that optimization, but it turns out it requires ReFinalize, so it was simpler to remove it; I want CodeFolding to avoid ReFinalize so the fuzzer shows it gets all the block types correct. Anyways, this output will be cleaned up by OptimizeInstructions, so I don't think this is a problem.

Copy link
Member

Choose a reason for hiding this comment

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

I see, got it. That makes sense. Maybe add that to the description/commit.

Though I think a few lines that turn (if X (then) (else) into (drop X) might be worth it for clarity and efficiency. That is, right after we optimize an if, we can see if the arms are empty and handle that. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, that sounds good.

@tlively tlively requested a review from kripken November 26, 2024 23:02
;; CHECK-NEXT: )
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: (i32.const 1)
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look right. Imagine the inner blocks are ifs, and that we get false in both conditions, so we enter neither. Then before, all we execute is one nop and then return the constant. But after, we execute two nops. (If the nops were say calls, then we'd have possibly different effects as a result.)

Copy link
Member

@kripken kripken Nov 26, 2024

Choose a reason for hiding this comment

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

Or we'd execute 3 nops after: one right after the two blocks (the blocks that we assume we skip), and two more outside. Or am I reading this wrong?

Copy link
Member Author

Choose a reason for hiding this comment

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

The correctness here depends on the fact that the inner blocks are unreachable. This means that the fallthrough tail cannot possibly be executed, so it is not considered when merging tails. This test therefore moves two nops instead of just one. The very next test is the same except that the blocks are not unreachable, so it considers the fallthrough tail and only moves one nop.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this special casing for blocks containing unreachable instructions existed before this PR, but was not tested, at least not in lit tests.

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks!

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Nice!

;; CHECK-NEXT: )
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: (i32.const 1)
Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks!

@tlively tlively merged commit 98f5798 into main Nov 27, 2024
13 checks passed
@tlively tlively deleted the code-folding-concrete branch November 27, 2024 00:04
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