[tlively]

Extend Properties::getSignExtValue, getSignExtBits, and getAlmostSignExt to 64-bit integers and unary extend{8,16,32}_s operations. Fold unary and shift-pair sign extensions of narrow loads into signed loads in visitUnary and visitBinary, and generalize Eq/Ne sign-extension comparisons to i64.

[gemini-code-assist]

Code Review

This pull request generalizes sign-extension optimizations in Binaryen to support both i32 and i64 types. It updates helper functions in src/ir/properties.h to handle arbitrary integer types and extends various optimizations in src/passes/OptimizeInstructions.cpp (such as folding sign-extensions into narrow loads and simplifying comparisons with constants) to work with i64 operations. Feedback on the changes highlights a critical crash risk in OptimizeInstructions.cpp where getSignExtBits can be called on a non-sign-extend expression, as well as a robustness issue in properties.h where shift amounts are compared literally instead of using their effective modulo values.

[gemini-code-assist]

The call to getSignExtBits(ext) is unsafe because ext (which is curr->value) is not guaranteed to be a sign-extend expression. For example, if curr is (i32.extend8_s (local.get $x)), ext will be (local.get $x). Since LocalGet is neither a Unary nor a Binary sign-extend, getSignExtBits will fall through to the else branch and attempt to perform curr->cast<Binary>() on the LocalGet, resulting in an assertion failure/crash.

To fix this, you should check if ext is actually a sign-extend expression before calling getSignExtBits(ext).

      if (Properties::getSignExtValue(ext)) {
        auto signExtBits = getSignExtBits(ext);
        if (signExtBits > 0 && signExtBits <= bits) {
          return replaceCurrent(ext);
        }
      }

[tlively]

/review

[kripken]

What is handled here that is not handled in the above appearance of this code (new line 614)?

[tlively]

Here we're optimizing the unary sign extension instructions. The previous location was optimizing shift-based sign extensions and almost-sign-extensions.

[kripken]

I see. Can they both use a helper, then? it seems like both can use the same logic?

[tlively]

Good idea. Done.

[kripken]

Can we not use getAlmostSignExtBits like the other call to this helper? If not, why must we be more precise here?

[tlively]

Two things:

1. The other call is guarded by extraLeftShifts == 0, so it is also looking at an exact sign extension. The only reason there is a Properties::getAlmostSignExtBits call is because if there are extra left shifts, it falls through and tries other optimizations. The trySignExtLoad optimization only works for exact sign extensions because it is folding the sign extension into a sign-extending load.
2. getAlmostSignExt and getAlmostSignExtendBits only work with shift-based sign extensions right now. We could generalize them, but there wouldn't be any benefit in this PR due to the previous point.

[kripken]

Suggested change

	;; Comparing i64 sign-extension against signed load on right or left.
	;; Comparing i64 sign-extension against signed load on right or left.
	;; The upper 32 bits are sign bits, and equal iff the lower bits are, so
	;; we can just do an unsigned comparison on the lower 32.

Is that right?

[tlively]

yes, exactly.

[kripken]

Why do we do this transform? It is larger, and I don't see an obvious reason why it would be faster?

[tlively]

Good question. According to some AI Overview research, the 32-to-64-bit zero extension (i.e. this masking) can be slightly cheaper than the corresponding sign extension on x64, but that is only one case and depends on V8 recognizing that the mask can be folded into other instructions (I don't know whether it recognizes that). In all other cases and on Arm64, they appear to have the same performance.

However, I'm pretty sure this is a pre-existing optimization: https://github.com/WebAssembly/binaryen/pull/9296/changes#diff-d03831e8b5cc4d080b5884abbf6d6d2776449ca53b3655a2cd5a07e57d6a11c5R676-L694. The only new functionality tested here is that the existing optimization now applies to i64 sign extensions in addition to i32 sign extensions. Maybe we should remove that optimization entirely?

[kripken]

Hmm, I am having trouble opening that link. What file and line should it be pointing to? (I looked at line 694 based on the URL, in the big cpp file, but it is a closing brace so that's not it)

[kripken]

(I tried opening it in chrome and firefox, same issue)

[tlively]

OptimizeInstructions.cpp, new lines 676 to 694.

[kripken]

I see, thanks. Well, as it is pre-existing, let's leave it for later.