[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.

gemini-code-assist · 2026-10-29T04:58:28Z

+      // A narrower outer shift sign-extension can strip a wider inner one.
+      if (auto* innerExt = Properties::getSignExtValue(ext)) {
+        if (bits - extraLeftShifts <= Properties::getSignExtBits(ext)) {
+          curr->left->cast<Binary>()->left = innerExt;


Since left is already bound to curr->left as a Binary* in the outer scope of this block, you can directly assign to left->left instead of performing a redundant cast on curr->left.

left->left = innerExt;

This does not appear to be correct.

…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)) ) )

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)

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

      // We also don't need it if it already has an identical-sized sign
      // extend applied to it. That is, if it is already a sign-extended


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

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

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>()) {

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

@@ -615,10 +635,22 @@
      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.

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)

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

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

Navigation Menu

Repository navigation

[OptimizeInstructions] Remove redundant sign-exts - #9324

[OptimizeInstructions] Remove redundant sign-exts#9324
tlively wants to merge 3 commits into
mainfrom
opt-inst-1-3-redundant-sign-ext

tlively commented Oct 29, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot Oct 29, 2026

Uh oh!

tlively Oct 29, 2026

Uh oh!

aheejin left a comment

Uh oh!

aheejin Oct 31, 2026

Uh oh!

aheejin Oct 31, 2026 •

edited

Loading

Uh oh!

aheejin Oct 31, 2026

Uh oh!

aheejin Nov 1, 2026

Uh oh!

aheejin Nov 1, 2026

Uh oh!

aheejin Nov 1, 2026

Uh oh!

aheejin Nov 1, 2026 •

edited

Loading

Uh oh!

aheejin Nov 1, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

		// We also don't need it if it already has an identical-sized sign
		// extend applied to it. That is, if it is already a sign-extended

	// 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 @@
		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!

gemini-code-assist Bot Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

tlively Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

aheejin left a comment

Choose a reason for hiding this comment

Uh oh!

aheejin Oct 31, 2026

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!

aheejin Oct 31, 2026

Choose a reason for hiding this comment

Uh oh!

aheejin Nov 1, 2026

Choose a reason for hiding this comment

Uh oh!

aheejin Nov 1, 2026

Choose a reason for hiding this comment

Uh oh!

aheejin Nov 1, 2026

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!

aheejin Nov 1, 2026

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