Skip to content

Commit

Permalink
OptimizeInstructions: Add missing invalidation check in consecutive e…
Browse files Browse the repository at this point in the history
…quality test (#6596)

This existed before #6495 but became noticeable there. We only looked at
the fallthrough values in the later part of areConsecutiveInputsEqual, but
there can be invalidation due to the non-fallthrough part:

(i32.add
  (local.get $x)
  (block
    (local.set $x ..)
    (local.get $x)
  )
)

The set can cause the local.get to differ the second time. To fix this,
check if the non-fallthrough part invalidates the fallthrough (but only
on the right hand side).

Fixes #6593
  • Loading branch information
kripken committed May 15, 2024
1 parent ae94994 commit 2cc5e06
Show file tree
Hide file tree
Showing 2 changed files with 267 additions and 17 deletions.
22 changes: 22 additions & 0 deletions src/passes/OptimizeInstructions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2501,11 +2501,33 @@ struct OptimizeInstructions
// Ignore extraneous things and compare them syntactically. We can also
// look at the full fallthrough for both sides now.
left = Properties::getFallthrough(left, passOptions, *getModule());
auto* originalRight = right;
right = Properties::getFallthrough(right, passOptions, *getModule());
if (!ExpressionAnalyzer::equal(left, right)) {
return false;
}

// We must also not have non-fallthrough effects that invalidate us, such as
// this situation:
//
// (local.get $x)
// (block
// (local.set $x ..)
// (local.get $x)
// )
//
// The fallthroughs are identical, but the set may cause us to read a
// different value.
if (originalRight != right) {
// TODO: We could be more precise here and ignore right itself in
// originalRightEffects.
auto originalRightEffects = effects(originalRight);
auto rightEffects = effects(right);
if (originalRightEffects.invalidates(rightEffects)) {
return false;
}
}

// To be equal, they must also be known to return the same result
// deterministically.
return !Properties::isGenerative(left);
Expand Down

0 comments on commit 2cc5e06

Please sign in to comment.