Skip to content

Use empty blocks instead of nops for empty scopes in IRBuilder #7080

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

Merged
merged 4 commits into from
Nov 15, 2024

Conversation

tlively
Copy link
Member

@tlively tlively commented Nov 15, 2024

When IRBuilder builds an empty non-block scope such as a function body,
an if arm, a try block, etc, it needs to produce some expression to
represent the empty contents. Previously it produced a nop, but change
it to produce an empty block instead. The binary writer and printer have
special logic to elide empty blocks, so this produces smaller output.

Update J2CLOpts to recognize functions containing empty blocks as
trivial to avoid regressing one of its tests.

When IRBuilder builds an empty non-block scope such as a function body,
an if arm, a try block, etc, it needs to produce some expression to
represent the empty contents. Previously it produced a nop, but change
it to produce an empty block instead. The binary writer and printer have
special logic to elide empty blocks, so this produces smaller output.

Update J2CLOpts to recognize functions containing empty blocks as
trivial to avoid regressing one of its tests.
@tlively tlively requested a review from kripken November 15, 2024 00:26
@@ -33,7 +33,8 @@
;; CHECK-NEXT: (local.set $0
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: (block
;; 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.

These nested locations surprise me a little. Was the old parser really emitting an empty block there? I am pretty sure it wasn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think what's happening here is that the empty function gets parsed with an empty block and then DAE adds the local.set in front of the empty block rather than inside it. Since the empty block is no longer the function body, the printer happily starts printing it.

I do know that when we switched over to the IRBuilder-based text parser, we ended up adding a lot of nops to a lot of tests, but I don't think the old text parser could have been doing anything different in this case, since it would have had to do something or other for the empty function body.

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. Seems reasonable.

Maybe worth checking if say the unoptimized code size tests in Emscripten change with this, just to be safe,

./test/runner other.test*unoptimized*

(optimized size should not be able to change as empty blocks get removed).

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, don't those tests skip Binaryen entirely because they are run with -O0?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also this only affects the text parser for now.

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, nevermind my worries then.

@tlively tlively merged commit 49c45ac into main Nov 15, 2024
13 checks passed
@tlively tlively deleted the ir-builder-empty-block branch November 15, 2024 02:22
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