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

Add syntax for cold blocks to CLIF. #3708

Merged
merged 1 commit into from
Jan 21, 2022
Merged

Add syntax for cold blocks to CLIF. #3708

merged 1 commit into from
Jan 21, 2022

Conversation

cfallin
Copy link
Member

@cfallin cfallin commented Jan 20, 2022 •
edited
Loading

This commit adds support for denoting cold blocks in the CLIF text
format as follows:


function %f() {
block0(...):
  ...

block1 cold:
  ...

block2(...) cold:
  ...

block3:
  ...

With this syntax, we are able to see the cold-block flag in CLIF, we can
write tests using it, and it is preserved when round-tripping.

Fixes #3701.

@bjorn3
Copy link
Contributor

bjorn3 commented Jan 20, 2022

Maybe placing it just before the : is nicer? For example block0(v0: i8) cold:. This would avoid ambiguity if we get more block flags like to indicate landingpads for unwinding. It would also extend better to marking whole functions as cold if we get an inliner or change the way calls are codegened if the callee is marked as cold.

@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Jan 21, 2022
cfallin added a commit to cfallin/wasmtime that referenced this pull request Jan 21, 2022
@cfallin
Cranelift: Fix cold-blocks-related lowering bug.
2418e59 
If a block is marked cold but has side-effect-free code that is only
used by side-effectful code in non-cold blocks, we will erroneously fail
to emit it, causing a regalloc failure.

This is due to the interaction of block ordering and lowering: we rely
on block ordering to visit uses before defs (except for backedges) so
that we can effectively do an inline liveness analysis and skip lowering
operations that are not used anywhere. This "inline DCE" is needed
because instruction lowering can pattern-match and merge one instruction
into another, removing the need to generate the source instruction.

Unfortunately, the way that I added cold-block support in bytecodealliance#3698 was
oblivious to this -- it just changed the block sort order. For
efficiency reasons, we generate code in its final order directly, so it
would not be tenable to generate it in e.g. RPO first and then reorder
cold blocks to the bottom; we really do want to visit in the same order
as the final code.

This PR fixes the bug by moving the point at which cold blocks are sunk
to emission-time instead. This is cheaper than either trying to visit
blocks during lowering in RPO but add to VCode out-of-order, or trying
to do some expensive analysis to recover proper liveness. It's not clear
that the latter would be possible anyway -- the need to lower some
instructions depends on other instructions' isel results/merging
success, so we really do need to visit in RPO, and we can't simply lower
all instructions as side-effecting roots (some can't be toplevel nodes).

The one downside of this approach is that the VCode itself still has
cold blocks inline; so in the text format (and hence compile-tests) it's
not possible to see the sinking. This PR adds a test for cold-block
sinking that actually verifies the machine code. (The test also includes
an add-instruction in the cold path that would have been incorrectly
skipped prior to this fix.)

Fortunately this bug would not have been triggered by the one current
use of cold blocks in bytecodealliance#3699, because there the only operation in the
cold block was an (always effectful) call instruction. The worst-case
effect of the bug in other code would be a regalloc panic; no silent
miscompilations could result.

Depends on bytecodealliance#3708.
cfallin added a commit to cfallin/wasmtime that referenced this pull request Jan 21, 2022
@cfallin
Cranelift: Fix cold-blocks-related lowering bug.
06a58e4 
If a block is marked cold but has side-effect-free code that is only
used by side-effectful code in non-cold blocks, we will erroneously fail
to emit it, causing a regalloc failure.

This is due to the interaction of block ordering and lowering: we rely
on block ordering to visit uses before defs (except for backedges) so
that we can effectively do an inline liveness analysis and skip lowering
operations that are not used anywhere. This "inline DCE" is needed
because instruction lowering can pattern-match and merge one instruction
into another, removing the need to generate the source instruction.

Unfortunately, the way that I added cold-block support in bytecodealliance#3698 was
oblivious to this -- it just changed the block sort order. For
efficiency reasons, we generate code in its final order directly, so it
would not be tenable to generate it in e.g. RPO first and then reorder
cold blocks to the bottom; we really do want to visit in the same order
as the final code.

This PR fixes the bug by moving the point at which cold blocks are sunk
to emission-time instead. This is cheaper than either trying to visit
blocks during lowering in RPO but add to VCode out-of-order, or trying
to do some expensive analysis to recover proper liveness. It's not clear
that the latter would be possible anyway -- the need to lower some
instructions depends on other instructions' isel results/merging
success, so we really do need to visit in RPO, and we can't simply lower
all instructions as side-effecting roots (some can't be toplevel nodes).

The one downside of this approach is that the VCode itself still has
cold blocks inline; so in the text format (and hence compile-tests) it's
not possible to see the sinking. This PR adds a test for cold-block
sinking that actually verifies the machine code. (The test also includes
an add-instruction in the cold path that would have been incorrectly
skipped prior to this fix.)

Fortunately this bug would not have been triggered by the one current
use of cold blocks in bytecodealliance#3699, because there the only operation in the
cold block was an (always effectful) call instruction. The worst-case
effect of the bug in other code would be a regalloc panic; no silent
miscompilations could result.

Depends on bytecodealliance#3708.
@cfallin cfallin mentioned this pull request Jan 21, 2022
Merged
@cfallin
Add syntax for cold blocks to CLIF.
51649d5 
This commit adds support for denoting cold blocks in the CLIF text
format as follows:

```plain

function %f() {
block0(...):
  ...

block1 cold:
  ...

block2(...) cold:
  ...

block3:
  ...
```

With this syntax, we are able to see the cold-block flag in CLIF, we can
write tests using it, and it is preserved when round-tripping.

Fixes bytecodealliance#3701.
@cfallin
Copy link
Member Author

cfallin commented Jan 21, 2022

Sure, the parsing is a little more complex but it seems reasonable to be consistent with the way function flags work. Updated.

@cfallin cfallin requested a review from fitzgen January 21, 2022 00:52
@fitzgen fitzgen merged commit 78ff829 into bytecodealliance:main Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fitzgen fitzgen fitzgen approved these changes

@bjorn3 bjorn3 bjorn3 approved these changes

Assignees
No one assigned
Labels
cranelift Issues related to the Cranelift code generator
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cranelift: Print and parse cold flag for blocks
3 participants
@cfallin @bjorn3 @fitzgen