Fix mayNotReturn not checked in invalidates()

[sumleo]

Summary

EffectAnalyzer::invalidates() checks transfersControlFlow() to prevent reordering side-effecting expressions, but transfersControlFlow() does not include mayNotReturn. This means a potentially-infinite loop (which has mayNotReturn=true but transfersControlFlow()=false) can be incorrectly reordered with side-effecting code.

Concrete miscompilation: OptimizeInstructions flips select arms when the condition is i32.eqz, calling canReorder() to check safety. With a loop in one arm and a global.set in the other, canReorder() returns true (incorrectly), causing the global.set to always execute when it should only execute if the loop terminates.

;; Before (correct): global.set only runs if loop terminates
(select
  (block (result i32) (loop $L (br_if $L (local.get $x))) (i32.const 1))
  (block (result i32) (global.set $g (i32.const 42)) (i32.const 2))
  (i32.eqz (local.get $x)))

;; After optimize-instructions (WRONG): global.set always runs
(select
  (block (result i32) (global.set $g (i32.const 42)) (i32.const 2))
  (block (result i32) (loop $L (br_if $L (local.get $x))) (i32.const 1))
  (local.get $x))

Fix: Add mayNotReturn checks alongside the existing transfersControlFlow() checks in invalidates().

Test plan

• New lit test optimize-instructions-maynotreturn.wast covering:
  • Loop arm with global.set arm: arms NOT swapped
  • Loop arm with no-side-effect arm: arms still swapped (optimization preserved)
  • Loop arm with memory store arm: arms NOT swapped
• All 309 unit tests pass

[kripken]

While this is technically true in a way, I don't think we need to change things here.

The only way to observe the difference in the example given is if you run the program and pause it in a debugger or some other such tool, when it is infinite-looping. Pausing in a debugger can notice all sorts of other internal changes in the module, for example, reordering two local.sets leads to debugger-observable changes but not user-observable ones. Binaryen only preserves the latter - at least, I've not heard a strong enough reason for us to do anything more, and it has downsides.

Concretely, the infinite loop is exactly what prevents userspace from noticing a change to that global. There is simply no opportunity for other code to be influenced. (If the global were shared, an argument could be made that another thread could observe it, but cross-thread update timing is nondeterministic, so I'm not sure even then.)

With that said, this is non-obvious behavior, so (1) I'd be open to hearing other opinions, and (2) we should document this better.

[sumleo]

Thanks for the detailed explanation. That makes sense — an infinite loop prevents any downstream code from observing the global state change, so the reordering is semantically valid from the perspective of user-observable behavior. I hadn't considered that the non-termination itself acts as the observation barrier.

I'll close this PR. Thanks for taking the time to explain the reasoning.