[OptimizeInstructions] Remove redundant sign-exts by tlively · Pull Request #9324 · WebAssembly/binaryen

tlively · 2026-10-29T04:56:33Z

Relax isSignExted to check signExtBits <= bits, teach getSignExtBits about signed narrow loads and i64.extend_i32_s, remove unary sign-extensions when maxBits < bits, strip wider inner sign-extensions inside narrower outer ones, and fold i64.extend32_s(i64.extend_i32_u(x)) to i64.extend_i32_s(x).

gemini-code-assist

Code Review

This pull request enhances sign-extension optimizations in the OptimizeInstructions pass. It introduces a helper method getExpressionSignExtBits to track sign-extension widths, optimizes nested and redundant sign-extensions (both shift-based and unary), and simplifies expressions where the input's maximum bits are already within the target sign-extend limit. The feedback suggests a minor code cleanup to avoid a redundant cast by utilizing an already-bound pointer variable.

…t-sign-ext

aheejin

It would be easier to understand to have each if case comment in the code has a simple example attached to it.

aheejin · 2026-10-31T05:31:18Z

+    if (auto* unary = curr->dynCast<Unary>();
+        unary && unary->op == ExtendSInt32) {
+      // i64.extend_i32_s sign-extends from 32 bits, or from a narrower width if
+      // its i32 input is itself sign-extended (since any i32 sign extension is
+      // narrower than 32 bits and extending to i64 replicates the same sign
+      // bit).
+      bits = 32;
+      curr = unary->value;
+    }


This unwraps an outer i64.extend_i32_s if one exists and we can recover 16 in the if below. But maybe it'd be nice to handle this case too and get 8? Or would this be overkill?

(i64.extend_i32_s (i32.extend16_s (i32.extend8_s (x)) ) )

OptimizeInstructions optimizes in post-order and re-optimizes whenever an expression is replaced, so in this case the middle i32.extend16_s should already have been removed by the time we look at the outer i64.extend_i32_s. So looking deeper in this function wouldn't have much benefit.

aheejin · 2026-10-31T05:37:58Z

  Index getSignExtBits(Expression* curr) {
-    if (Properties::getSignExtValue(curr)) {
-      return Properties::getSignExtBits(curr);
+    if (auto* unary = curr->dynCast<Unary>();
+        unary && unary->op == ExtendSInt32) {
+      auto innerBits = getSignExtBits(unary->value);
+      return innerBits > 0 ? innerBits : 32;
    }


ExtendSInt32 check is also in LocalInfo::getExpressionSignExtBits, so I think we can skip that.

By the way can we merge this, LocalInfo::getExpressionSignExtBits, and Properties::getSignExtBits into one? We have three functions that do one task now. If having LocalInfo or not is a difference, we can provide it as a nullable option. We have a similar precedent already:

binaryen/src/ir/bits.h

Lines 164 to 166 in 677ad31

template<typename LocalInfoProvider = DummyLocalInfoProvider>

Index getMaxBits(Expression* curr,

LocalInfoProvider* localInfoProvider = nullptr) {

Can we merge all these into a single Properties::getSignExtBits?

(By the way, I'm new to this code, and getSignExtBits returning not the amount of sign-extended bits but the amount of 'kept' bits is really confusing... I'm not saying changing this is within the scope of this PR though)

Improved the names and consolidated helpers in #9350. I have restacked this PR on top of that one.

aheejin · 2026-10-31T22:29:49Z

It's not identical-sized anymore because isSignExted was relaxed

Updated the comment.

aheejin · 2026-11-01T20:58:29Z

        if (auto* ext = Properties::getSignExtValue(curr->left)) {
+          // TODO: Reexamine whether replacing unary sign-extend instructions
+          // (as opposed to shift pairs) with zero-extend masks here is
+          // worthwhile, since an and with a constant is larger than a unary


Suggested change

// worthwhile, since an and with a constant is larger than a unary

// worthwhile, since an 'and' with a constant is larger than a unary

Nit: This tripped my eyes several times because it looked like a conjunction... The same for other TODOs

Fixed with backticks.

aheejin · 2026-11-01T21:35:31Z

+        // TODO: Reexamine whether replacing unary sign-extend instructions (as
+        // opposed to shift pairs) with zero-extend masks in boolean contexts
+        // is worthwhile, since an and with a constant is larger than a unary
+        // extend.


Suggested change

// TODO: Reexamine whether replacing unary sign-extend instructions (as

// opposed to shift pairs) with zero-extend masks in boolean contexts

// is worthwhile, since an and with a constant is larger than a unary

// extend.

This is within else if (auto* binary = boolean->dynCast<Binary>()) {

Good catch. Removed.

aheejin · 2026-11-01T22:34:18Z

@@ -615,10 +635,22 @@ struct OptimizeInstructions
      if (isSignExted(ext, bits) && extraLeftShifts == 0) {


Suggested change

if (isSignExted(ext, bits) && extraLeftShifts == 0) {

if (isSignExted(ext, bits - extraLeftShifts)) {

This can unlock more cases, such as (extend8_s(x) << 16) >>_s 8.

Also if we do this, this case and the case below are representing the opposite cases: This case is when the inner is narrower and the case below is when the inner is wider. Would be nice to revise the comments to make it clear that these two cases are essentially the same thing with the inner and outer swapped.

Good idea, done.

aheejin · 2026-11-01T22:55:38Z

+      // having a larger sign-extend: if we have an 8-bit value in 32-bit, then
+      // there are 24 sign bits, and doing a sign-extend to 16 will only affect
+      // 16 of those 24, and the effect is to leave them as they are.
+      if (Bits::getMaxBits(ext, this) < bits || isSignExted(ext, bits)) {


Nit: To match the cases in visitBinary, to me it would be easier to split these to two cases

aheejin · 2026-11-01T22:58:58Z

+      // Sign-extending the low 32 bits of a zero-extended i32 is a signed i32
+      // extension.
+      Unary* inner;
+      if (matches(curr,
+                  unary(ExtendS32Int64, unary(&inner, ExtendUInt32, any())))) {
+        inner->op = ExtendSInt32;
+        return replaceCurrent(inner);
+      }


Can we add this case to visitBinary too, so that we can do these?

(i64.extend_i32_u(x) << 32) >>_s 32 ⟶ i64.extend_i32_s(x) (i64.extend_i32_u(x) << 32) >>_s 24 ⟶ i64.extend_i32_s(x) << 8 (when extraLeftShifts > 0)

Good catch. Done.

…t-sign-ext

Merge OptimizeInstructions::getSignExtBits into Properties::getSignExtBits using an optional LocalInfoProvider (following Bits::getMaxBits), and rename the sign/zero-extension bit width helpers from get*ExtBits to get*ExtKeptBits to clarify that they return the number of low bits kept before extension rather than the number of extended sign/zero bits.

# Conflicts: # src/passes/OptimizeInstructions.cpp

Merge OptimizeInstructions::getSignExtBits into Properties::getSignExtBits using an optional LocalInfoProvider (following Bits::getMaxBits), and rename the sign/zero-extension bit width helpers from get*ExtBits to get*ExtKeptBits to clarify that they return the number of low bits kept before extension rather than the number of extended sign/zero bits.

# Conflicts: # src/ir/properties.h # src/passes/OptimizeInstructions.cpp

…t-sign-ext # Conflicts: # src/ir/bits.h # src/ir/properties.h # src/passes/OptimizeInstructions.cpp # src/passes/PickLoadSigns.cpp

github-actions

🤖 Google Antigravity Code Review

I have reviewed the changes in this Pull Request and left inline feedback.

github-actions · 2026-11-07T00:52:21Z

  // If |curr| is known to be sign-extended, returns the number of low bits kept
  // before sign-extending into the upper bits, or 0 otherwise.
  Index getSignExtKeptBits(Expression* curr) const {
+    if (auto* unary = curr->dynCast<Unary>();


[HIGH | Confidence: 5/5 | BUG]

getSignExtKeptBits does not verify that curr->type.isInteger() or that unary->value->type.isInteger(). In Binaryen IR, an inner expression can be replaced with an unreachable node (such as a trapping expression) before refinalization updates the parent's type, or curr itself may have Type::unreachable (e.g., (i64.extend_i32_s (unreachable))).

Existing helper functions in bits.h explicitly handle this invariant (see getSignExtValue lines 153 and 163-165: The operand may have become unreachable before refinalization updates curr->type). Without checking unary->value->type.isInteger(), getSignExtKeptBits recurses on the unreachable child, gets innerBits == 0, and falls back to returning 32 for an unreachable node. This causes hasSignExtKeptBits and callers in optimization passes to incorrectly treat unreachable expressions as valid 32-bit integer sign-extensions.

Consider guarding the check:

Suggested change

if (auto* unary = curr->dynCast<Unary>();

if (auto* unary = curr->dynCast<Unary>();

unary && unary->op == ExtendSInt32 &&

curr->type.isInteger() && unary->value->type.isInteger()) {

auto innerBits = getSignExtKeptBits(unary->value);

return innerBits > 0 ? innerBits : 32;

}

Fixed with test.

tlively requested a review from a team as a code owner October 29, 2026 04:56

tlively requested review from aheejin and removed request for a team October 29, 2026 04:56

gemini-code-assist Bot reviewed Oct 29, 2026

View reviewed changes

Comment thread src/passes/OptimizeInstructions.cpp Resolved

tlively added 2 commits October 29, 2026 14:42

Merge remote-tracking branch 'origin/main' into opt-inst-1-3-redundan…

55c9b85

…t-sign-ext

update tests

8241334

aheejin reviewed Nov 1, 2026

View reviewed changes

tlively added 9 commits November 3, 2026 11:06

Merge remote-tracking branch 'origin/main' into opt-inst-1-3-redundan…

cda07d2

…t-sign-ext

Merge branch 'sign-ext-kept-bits' into opt-inst-1-3-redundant-sign-ext

88f82d7

# Conflicts: # src/passes/OptimizeInstructions.cpp

update comments

0052db2

Add missing opt and split conditions

58ed710

Merge branch 'sign-ext-kept-bits' into opt-inst-1-3-redundant-sign-ext

f8de505

# Conflicts: # src/ir/properties.h # src/passes/OptimizeInstructions.cpp

Generalize almost-sign-ext outer elimination

ca304db

Merge remote-tracking branch 'origin/main' into opt-inst-1-3-redundan…

2baf94f

…t-sign-ext # Conflicts: # src/ir/bits.h # src/ir/properties.h # src/passes/OptimizeInstructions.cpp # src/passes/PickLoadSigns.cpp

github-actions Bot reviewed Nov 7, 2026

View reviewed changes

guard against unreachable

071133e

	template<typename LocalInfoProvider = DummyLocalInfoProvider>
	Index getMaxBits(Expression* curr,
	LocalInfoProvider* localInfoProvider = nullptr) {

	// worthwhile, since an and with a constant is larger than a unary
	// worthwhile, since an 'and' with a constant is larger than a unary

		@@ -615,10 +635,22 @@ struct OptimizeInstructions
		if (isSignExted(ext, bits) && extraLeftShifts == 0) {

	if (isSignExted(ext, bits) && extraLeftShifts == 0) {
	if (isSignExted(ext, bits - extraLeftShifts)) {

Conversation

tlively commented Oct 29, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

aheejin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aheejin Oct 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aheejin Nov 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Nov 7, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aheejin Oct 31, 2026 •

edited

Loading

aheejin Nov 1, 2026 •

edited

Loading