RemoveUnusedBrs: optimize unreachable control flow mixed with side-effecting branches

[xuruiyang2002]

Currently wasm-opt cannot optimize unused branch complexed with side-effect operations, such as

 (func $_start
  (local $0 i32)
  (block $block
   (br_if $block
    (local.tee $0
     (i32.const 1)
    )
   )
   (br_if $block
    (i32.load
     (i32.const 0)
    )
   )
   (local.set $0
    (i32.const 0)
   )
  )
  (i32.store
   (i32.const 0)
   (local.get $0)
  )
 )

Actually, the whole block can be removed. However, O3 cannot optimize it (while O2 could).

Fixes: #7637

[xuruiyang2002]

The fuzzer found a bug here:

(module
  (type (;0;) (func (param i64)))
  (type (;1;) (func))
  (import "fuzzing-support" "log-i64" (func (;0;) (type 0)))
  (func (;1;) (type 1)
    (local i64 i32)
    block  ;; label = @1
      block  ;; label = @2
        global.get 0
        if  ;; label = @3
          i32.const 1
          local.tee 1
          if  ;; label = @4
            br 3 (;@1;)
          else
            br 2 (;@2;)
          end
          unreachable
        else
          unreachable
        end
        unreachable
      end
      local.get 0
      call 0
    end)
  (global (;0;) (mut i32) (i32.const 0)))

$ bin/wasm-opt b.wat -O1 -S -o -
wasm-opt: SimplifyLocals.cpp:666: void wasm::SimplifyLocals<allowTee, allowStructure, allowNesting>::optimizeIfElseReturn(wasm::If*, wasm::Expression**, Sinkables&) [with bool allowTee = true; bool allowStructure = true; bool allowNesting = true; Sinkables = std::map<unsigned int, wasm::SimplifyLocals<true, true>::SinkableInfo, std::less<unsigned int>, std::allocator<std::pair<const unsigned int, wasm::SimplifyLocals<true, true>::SinkableInfo> > >]: 
Assertion `iff->ifFalse->type != Type::unreachable' failed.
Aborted (core dumped)

[xuruiyang2002]

Because of this line curr->type = Type::unreachable; (which fixes type mismatch after transforming br_if to br)

As for the case found by the fuzzer, it transforms code from

    block  ;; label = @1
      global.get 0
      if  ;; label = @2
        i32.const 1
        br_if 1 (;@1;)
      else
        unreachable
      end
      local.get 0
      call 0
    end

to

    block  ;; label = @1
      global.get 0
      if  ;; label = @2
        br 1 (;@1;)
      else
        unreachable
      end
      local.get 0
      call 0
    end

which in SimplifyLocals triggers the assertion:

binaryen/src/passes/SimplifyLocals.cpp

Lines 659 to 666 in 700fa15

	if (iff->ifTrue->type == Type::unreachable) {
	// since the if type is none
	assert(iff->ifFalse->type != Type::unreachable);
	if (!ifFalse.empty()) {
	goodIndex = ifFalse.begin()->first;
	found = true;
	}
	} else if (iff->ifFalse->type == Type::unreachable) {

[xuruiyang2002]

Should this assertion always hold true?

[kripken]

Yes. If an if has two branches of unreachable then the if should be unreachable: logically, if neither if arm returns, neither does the if itself.

So we need to figure out how things got to that situation, and prevent it. Likely a refinalize() call is missing somewhere (which updates the types of expression after changes - that would update an if to unreachable if it needs to be, etc.)

[kripken]

Probably the issue is that turning a br_if into a br makes it unreachable, when it wasn't before. Setting refinalize = true; in that case should fix things.

[xuruiyang2002]

Got it, and I've updated.

And I've also fuzzing for about one hour; all is well.