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