[tlively]

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

[gemini-code-assist]

Code Review

This pull request refactors the tracking of sign-extended and zero-extended values by renaming and updating functions to focus on 'kept bits' (the number of low bits preserved before extension). It introduces a templated LocalInfoProvider mechanism in getSignExtKeptBits to allow querying local variables (local.get) for their sign-extension status. Corresponding updates are made across OptimizeInstructions and PickLoadSigns passes to use the new getSignExtKeptBits, getAlmostSignExtKeptBits, and getZeroExtKeptBits interfaces. I have no feedback to provide on these changes.

[stevenfontanella]

I wonder if we considered putting this in a class? Since this and getMaxBits are both taking a dependency that could be mocked out (with DummyLocalInfoProvider). In which case we'd use an abstract class for LocalInfoProvider instead of a template

[tlively]

Can you sketch what you have in mind more concretely?

[stevenfontanella]

Something like

class ArithmeticInterpreter {
  ArithmeticInterpreter(LocalInfoProvider* localInfoProvider) {}

  Index getMaxBits(Expression* curr) const;
  Index getSignExtKeptBits(Expression* curr) const;
  // maybe more methods in the future
};

[tlively]

Ok, nice idea. I've done that.

[aheejin]

Are we not gonna delete this?

binaryen/src/passes/OptimizeInstructions.cpp

Lines 4451 to 4462 in a9141c8

	// Returns the number of bits an expression is sign-extended (or 0 if it is
	// not).
	Index getSignExtBits(Expression* curr) {
	if (Properties::getSignExtValue(curr)) {
	return Properties::getSignExtBits(curr);
	}
	if (auto* get = curr->dynCast<LocalGet>()) {
	// Check what we know about the local.
	return localInfo[get->index].signExtBits;
	}
	return 0;
	}

[aheejin]

Can't we use Properties::getSignExtKeptBits here?

[tlively]

No, because that only returns results for exact sign extensions, not the "almost" sign extensions we are handling here.

[tlively]

Hold on, I'm going to try to make the diff smaller.

[tlively]

Are we not gonna delete this?

It is deleted now.

[gemini-code-assist]

Code Review

This pull request refactors bit-tracking utilities by moving sign-extend and zero-extend helper functions from Properties to Bits and introducing a Bits::Provider class that utilizes a LocalInfoProvider interface. The review feedback recommends optimizing the Provider class by storing localInfoProvider as a pointer instead of a reference to avoid virtual call overhead on the dummy fallback, adding corresponding null checks when querying the provider, and renaming a shadowed local variable bits in OptimizeInstructions.cpp to improve code readability.

[gemini-code-assist]

Using a reference for localInfoProvider forces a virtual function call on the fallback dummyLocalInfoProvider even when no provider is needed (which is the default and most common case). Changing localInfoProvider to a pointer and checking for nullptr avoids this virtual call overhead in the hot path of getMaxBits and eliminates the need for the static inline const fallback instance.

Suggested change

	class Provider {
	const LocalInfoProvider& localInfoProvider;
	static inline const DummyLocalInfoProvider dummyLocalInfoProvider{};
	public:
	Provider(const LocalInfoProvider* localInfoProvider = nullptr)
	: localInfoProvider(localInfoProvider ? *localInfoProvider
	: dummyLocalInfoProvider) {}
	class Provider {
	const LocalInfoProvider* localInfoProvider;
	public:
	Provider(const LocalInfoProvider* localInfoProvider = nullptr)
	: localInfoProvider(localInfoProvider) {}

[gemini-code-assist]

Update the LocalGet handling to check if localInfoProvider is non-null before calling its method, falling back to direct type-based bit width checks to avoid virtual call overhead.

    } else if (auto* get = curr->dynCast<LocalGet>()) {
      if (localInfoProvider) {
        return localInfoProvider->getMaxBitsForLocal(get);
      }
      if (get->type == Type::i32) {
        return 32;
      } else if (get->type == Type::i64) {
        return 64;
      }
      WASM_UNREACHABLE("type has no integer bit size");

[tlively]

This seems worse. I verified locally that the performance difference of using virtual calls is negligible.

[gemini-code-assist]

Update the LocalGet handling in getSignExtKeptBits to safely handle the null localInfoProvider pointer.

Suggested change

	if (auto* get = curr->dynCast<LocalGet>()) {
	return localInfoProvider.getSignExtKeptBitsForLocal(get);
	}
	if (auto* get = curr->dynCast<LocalGet>()) {
	return localInfoProvider ? localInfoProvider->getSignExtKeptBitsForLocal(get) : 0;
	}

[gemini-code-assist]

The local variable bits shadows the member variable bits of OptimizeInstructions, forcing the use of this->bits. Renaming the local variable to keptBits avoids this shadowing and improves code readability.

    if (auto* ext = Bits::getAlmostSignExt(curr)) {
      Index extraLeftShifts;
      auto keptBits = Bits::getAlmostSignExtKeptBits(curr, extraLeftShifts);
      if (extraLeftShifts == 0 && trySignExtLoad(ext, keptBits)) {
        return replaceCurrent(ext);
      }
      // We can in some cases remove part of a sign extend, that is,
      //   (x << A) >> B   =>   x << (A - B)
      // If the sign-extend input cannot have a sign bit, we don't need it.
      if (bits.getMaxBits(ext) + extraLeftShifts < keptBits) {
        return replaceCurrent(removeAlmostSignExt(curr));
      }
      // 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
      // value, then another sign extend will do nothing. We do need to be
      // careful of the extra shifts, though.
      if (hasSignExtKeptBits(ext, keptBits) && extraLeftShifts == 0) {
        return replaceCurrent(removeAlmostSignExt(curr));
      }

[tlively]

Done.

[aheejin]

The code changes got a lot bigger than yesterday. What changed?

[tlively]

The biggest contributor was that I moved the sign-extension functionality from properties.h to ir/bits.h. @stevenfontanella's idea to use a class with virtual methods instead of templates to provide local information also caused quite a few mechanical changes.