Skip to content

getMaxBits fails to analyze Block expression, missing i32.and -> 0 optimization #7559

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
xuruiyang2002 opened this issue Apr 27, 2025 · 1 comment · May be fixed by #7590
Open

getMaxBits fails to analyze Block expression, missing i32.and -> 0 optimization #7559

xuruiyang2002 opened this issue Apr 27, 2025 · 1 comment · May be fixed by #7590

Comments

@xuruiyang2002
Copy link
Contributor

Given the following code:

(module
  (import "External" "external_function" (func $external_function))
  (func $foo (param $0 i32) (param $1 i32) (result i32)
    i32.const 0)
  (func $_start (param $0 i32) (param $1 i64) (param $2 i32) (param $3 i32)
    i32.const 0
    i32.load
    local.set $0
    local.get $0
    local.set $2
    i32.const 0
    i32.const 0
    call $foo
    local.set $0
    block ;; label = @1
      local.get $0
      br_if 0 (;@1;)
      i32.const 0
      i32.const 0
      i32.store
      i32.const 0
      local.set $3
    end
    local.get $3
    local.set $0
    local.get $2
    local.get $0
    i32.and
    local.set $0
    block ;; label = @1
      local.get $0
      i32.eqz
      br_if 0 (;@1;)
      call $external_function
    end)
  (memory $0 258 258)
  (export "_start" (func $_start)))

wasm-opt (e6d02fa) by -all -O3 cannot deduce the condition

   (i32.and
    (i32.load
     (i32.const 0)
    )
    (block (result i32)
     (i32.store
      (i32.const 0)
      (i32.const 0)
     )
     (i32.const 0)
    )

to zero (but -all -O2 can), thus misses an optimization opportunity.

Similar to #7455 and #7556, I think the probably cause is the limitation of merge-blocks, which cannot hoist the constant in the block out due to the load and store instructions cannot reordered. Below the is logic code checking whether a given binary operation emits zero bits:

binaryen/src/passes/OptimizeInstructions.cpp

Lines 317 to 327 in e6d02fa

Expression* replaceZeroBitsWithZero(Expression* curr) {
// We should never be called with a constant.
assert(!curr->is<Const>());
if (!curr->type.isInteger() || Bits::getMaxBits(curr, this) != 0) {
return nullptr;
}
auto zero = Builder(*getModule()).makeConst(Literal::makeZero(curr->type));
return getDroppedChildrenAndAppend(curr, zero);
}

Currently the above logic cannot deduce the condition above to zero, since the getMaxBits does not handle the Block expression. Do you think there is a need to extend the getMaxBits to handle the Block?
@kripken
Copy link
Member

kripken commented Apr 29, 2025

Do you think there is a need to extend the getMaxBits to handle the Block?

Yes, I think that makes sense to do.

It looks like getMaxBits does look through the value of a LocalSet, and it could look at other fallthrough values. That is, we can remove the handling of LocalSet, and use getFallthrough at the start of the function (to handle LocalSet and all other things).

(That will require a change to the getMaxBits API, since it will need the pass options and the module, to pass to getFallthrough. But that seems worthwhile.)

@xuruiyang2002 xuruiyang2002 linked a pull request May 14, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Extend getMaxbits to handle Block type xuruiyang2002/binaryen
2 participants
@kripken @xuruiyang2002