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

Do not code fold ifs with concrete arms #7110

Closed
wants to merge 1 commit into from
+179 −111
Closed

Do not code fold ifs with concrete arms #7110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/passes/CodeFolding.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,10 @@ struct CodeFolding : public WalkerPass<ControlFlowWalker<CodeFolding>> {
if (!curr->ifFalse) {
return;
}
if (curr->ifTrue->type.isConcrete()) {
// We don't support folding tails that produce values.
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

We don't support that for brs, but for ifs I think we can? New line 259 has the logic to keep the concrete type the same.

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 comment on lines 502 to 508 explains one of the places that assumes we never have to handle folding concrete types.

The comment // we must ensure we present the same type as the if had on line 258 is also wrong; if an if is unreachable because of an unreachable condition, the ifTrue arm might have some other type.

Copy link
Member

Choose a reason for hiding this comment

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

The comment on line 502 looks correct to me, but it relates to blocks and brs, and not ifs, unless I'm confused?

Good point about unreachability, it is possible we aren't handling that well enough here. But I think we can add that without removing this optimization.

But with that said, I see we have that merging of arms logic in OptimizeInstructions:

binaryen/src/passes/OptimizeInstructions.cpp

Line 1140 in 485ac7d

// The sides are identical, so fold. If we can replace the If with one

So this looks like a redundant optimization actually, and perhaps we can remove it from here. But it is possible we don't handle unreachability fully there, so that might need fixing.

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 merging of if arms is handled by the same code that optimizes blocks and brs, so it’s not really possible that we support concrete types for one and not the others. It just happened to work out by accident until now because of the extra refinalization.

I’ll look into whether we can remove the optimizations of Ifs here completely or support concrete types for all folded branches.

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 don't think the folding in OptimizeInstructions can completely replace the optimizations here because it only works if the arms are completely identical, whereas this pass only requires identical suffixes.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, it looks like line 251 compares the entire ifTrue and ifFalse arms, while 263+ looks at suffixes. Was your change needed for one of the two?

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 test introduced here exposed bugs in the suffix folding code (in the presence of relaxed unreachable ifs), but #7094 also has to fix a bug with the folding of equivalent arms, so both kinds of If optimizations here are broken in the presence of concrete arms.

Copy link
Member

Choose a reason for hiding this comment

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

The new test below has unreachability, and I agree we should fix that handling here if needed.

What I don't follow is why we need to stop optimizing reachable cases. The first part of the test diff (where my comment is) looks like a regression. Can we not avoid that?

// if both sides are identical, this is easy to fold
if (ExpressionAnalyzer::equal(curr->ifTrue, curr->ifFalse)) {
Builder builder(*getModule());
Expand Down
63 changes: 55 additions & 8 deletions test/lit/passes/code-folding_enable-threads.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,18 @@
)
)
;; CHECK: (func $negative-zero-b (result f32)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (if (result f32)
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: (block $label$0 (result f32)
;; CHECK-NEXT: (f32.const -0)
;; CHECK-NEXT: (then
;; CHECK-NEXT: (block $label$0 (result f32)
;; CHECK-NEXT: (f32.const -0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (else
;; CHECK-NEXT: (block $label$1 (result f32)
;; CHECK-NEXT: (f32.const -0)
;; CHECK-NEXT: )
;; 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 was valid before, so this looks like a regression to me? I may be missing the reason you say this change is needed, sorry.

;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $negative-zero-b (result f32)
Expand All @@ -106,11 +113,18 @@
)
)
;; CHECK: (func $negative-zero-c (result f32)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (if (result f32)
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: (block $label$0 (result f32)
;; CHECK-NEXT: (f32.const 0)
;; CHECK-NEXT: (then
;; CHECK-NEXT: (block $label$0 (result f32)
;; CHECK-NEXT: (f32.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (else
;; CHECK-NEXT: (block $label$1 (result f32)
;; CHECK-NEXT: (f32.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $negative-zero-c (result f32)
Expand Down Expand Up @@ -482,3 +496,36 @@
)
)
)

(module
;; CHECK: (type $0 (func))

;; CHECK: (func $unreachable-if-concrete-arms
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (if (result i32)
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: (then
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: (else
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
(func $unreachable-if-concrete-arms
(if (result i32)
(unreachable)
(then
(i32.const 1)
)
(else
(nop)
(i32.const 1)
)
)
(unreachable)
)
)
53 changes: 27 additions & 26 deletions test/lit/passes/inlining-optimizing_optimize-level=3.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5742,43 +5742,44 @@
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (if
;; CHECK-NEXT: (i32.and
;; CHECK-NEXT: (local.get $9)
;; CHECK-NEXT: (i32.const 8)
;; CHECK-NEXT: )
;; CHECK-NEXT: (then
;; CHECK-NEXT: (local.set $7
;; CHECK-NEXT: (local.set $5
;; CHECK-NEXT: (if (result i32)
;; CHECK-NEXT: (i32.and
;; CHECK-NEXT: (local.get $9)
;; CHECK-NEXT: (i32.const 8)
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $6
;; CHECK-NEXT: (select
;; CHECK-NEXT: (local.tee $5
;; CHECK-NEXT: (i32.add
;; CHECK-NEXT: (i32.sub
;; CHECK-NEXT: (local.get $23)
;; CHECK-NEXT: (local.get $8)
;; CHECK-NEXT: (then
;; CHECK-NEXT: (local.set $7
;; CHECK-NEXT: (local.get $9)
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $6
;; CHECK-NEXT: (select
;; CHECK-NEXT: (local.tee $5
;; CHECK-NEXT: (i32.add
;; CHECK-NEXT: (i32.sub
;; CHECK-NEXT: (local.get $23)
;; CHECK-NEXT: (local.get $8)
;; CHECK-NEXT: )
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.get $6)
;; CHECK-NEXT: (i32.gt_s
;; CHECK-NEXT: (local.get $5)
;; CHECK-NEXT: (local.get $6)
;; CHECK-NEXT: (i32.gt_s
;; CHECK-NEXT: (local.get $5)
;; CHECK-NEXT: (local.get $6)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.get $8)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (else
;; CHECK-NEXT: (local.set $7
;; CHECK-NEXT: (local.get $9)
;; CHECK-NEXT: (else
;; CHECK-NEXT: (local.set $7
;; CHECK-NEXT: (local.get $9)
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.get $8)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $5
;; CHECK-NEXT: (local.get $8)
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $8
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: )
Expand Down
170 changes: 93 additions & 77 deletions test/passes/remove-unused-names_code-folding.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,19 @@
)
)
(drop
(block (result i32)
(drop
(i32.const 0)
(if (result i32)
(i32.const 0)
(then
(i32.add
(i32.const 1)
(i32.const 2)
)
)
(i32.add
(i32.const 1)
(i32.const 2)
(else
(i32.add
(i32.const 1)
(i32.const 2)
)
)
)
)
Expand Down Expand Up @@ -218,54 +224,61 @@
(unreachable)
)
(drop
(block (result i32)
(if
(i32.const 2)
(then
(drop
(i32.const -1234)
)
(drop
(i32.const -1000)
)
(if (result i32)
(i32.const 2)
(then
(drop
(i32.const -1234)
)
(else
(drop
(i32.const 999)
)
(drop
(i32.const -1000)
)
(drop
(i32.const 1)
)
(nop)
(unreachable)
(i32.const 2)
)
(drop
(i32.const 1)
(else
(drop
(i32.const 999)
)
(drop
(i32.const 1)
)
(nop)
(unreachable)
(i32.const 2)
)
(nop)
(unreachable)
(i32.const 2)
)
)
(drop
(block (result i32)
(if
(i32.const 3)
(then
(drop
(i32.const -1234)
)
(drop
(i32.const -1000)
)
(if (result i32)
(i32.const 3)
(then
(drop
(i32.const -1234)
)
(else
(drop
(i32.const 999)
)
(drop
(i32.const -1000)
)
(drop
(i32.const 1)
)
(nop)
(i32.const 2)
)
(drop
(i32.const 1)
(else
(drop
(i32.const 999)
)
(drop
(i32.const 1)
)
(nop)
(i32.const 2)
)
(nop)
(i32.const 2)
)
)
)
Expand Down Expand Up @@ -388,28 +401,28 @@
)
(drop
(block (result i32)
(block (result i32)
(if
(i32.const 9999)
(then
(drop
(i32.const -51234)
)
(drop
(i32.const -51000)
)
(if (result i32)
(i32.const 9999)
(then
(drop
(i32.const -51234)
)
(else
(drop
(i32.const 5999)
)
(drop
(i32.const 51)
)
(drop
(i32.const -51000)
)
(unreachable)
(i32.const 10)
)
(else
(drop
(i32.const 5999)
)
(drop
(i32.const 51)
)
(unreachable)
(i32.const 10)
)
(unreachable)
(i32.const 10)
)
)
)
Expand Down Expand Up @@ -1523,11 +1536,15 @@
)
(nop)
(drop
(block (result i32)
(drop
(unreachable)
(if (result i32)
(unreachable)
(then
(i32.add
(i32.const 1)
(i32.const 2)
)
)
(block (result i32)
(else
(i32.add
(i32.const 1)
(i32.const 2)
Expand Down Expand Up @@ -1874,20 +1891,19 @@
(i32.const 1)
)
)
(block (result i32)
(if
(local.get $x)
(then
)
(else
(drop
(call $if-suffix
(i32.const -2)
)
(if (result i32)
(local.get $x)
(then
(i32.const 2)
)
(else
(drop
(call $if-suffix
(i32.const -2)
)
)
(i32.const 2)
)
(i32.const 2)
)
)
)
Loading