Skip to content

LICM: Migrate from invalidates to orderedBefore #8743

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

Merged
tlively merged 4 commits into from
May 21, 2026
+165 −16
Merged

LICM: Migrate from invalidates to orderedBefore #8743

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
43b0811
LICM: Migrate from invalidates to orderedBefore
tlively May 20, 2026
20be375
LICM: Add regression tests for local variable tracking bugs
tlively May 21, 2026
970df3c
simplfy fix and improve docs
tlively May 21, 2026
42121ef
loops in comment
tlively May 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 11 additions & 10 deletions src/ir/effects.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,10 +303,8 @@ class EffectAnalyzer {
// (e.g., if we write, we must remain ordered before someone that reads).
//
// This assumes the things whose effects we are comparing will both execute,
// at least if neither of them transfers control flow away. That is, we assume
// that there is no transfer of control flow *between* them: we are comparing
// things appear after each other, perhaps with some other code in the middle,
// but that code does not transfer control flow. It is not valid to call this
// at least if neither of them transfers control flow away. We assume there is
// no transfer of control flow *between* them. It is not valid to call this
// method in other situations, like this:
//
// A
Expand All @@ -326,12 +324,15 @@ class EffectAnalyzer {
// ;; control flow transfer
// B
//
// That the things being compared both execute only matters in the case of
// traps-never-happen: in that mode we can move traps but only if doing so
// would not make them start to appear when they did not. In the second
// example we can't reorder A and B if B traps, but in the first example we
// can reorder them even if B traps (even if A has a global effect like a
// global.set, since we assume B does not trap in traps-never-happen).
// (Note that if they appear in inside a loop, A and B may overlap or even be
// the same expression; this is fine because A still executes before B, even
// if also executes during and after B across different loop iterations.) That
// A and B both execute only matters in the case of traps-never-happen: in
// that mode we can move traps but only if doing so would not make them start
// to appear when they did not previously. In the example with the br_if we
// can't reorder A and B if B traps, but in the valid examples we can reorder
// them even if B traps (even if A has a global effect like a global.set,
// since we assume B does not trap in traps-never-happen).
bool orderedBefore(const EffectAnalyzer& other) const {
// Cannot reorder control flow and side effects.
if ((transfersControlFlow() && other.hasSideEffects()) ||
Expand Down
10 changes: 6 additions & 4 deletions src/passes/LoopInvariantCodeMotion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,14 @@ struct LoopInvariantCodeMotion
// is ok to do so.
EffectAnalyzer effectsSoFar(getPassOptions(), *getModule());
// The loop's total effects also matter. For example, a store
// in the loop means we can't move a load outside.
// in the loop means we can't move a load outside. We discard the local
// reads and writes because we analyze them separately.
// FIXME: we look at the loop "tail" area too, after the last
// possible branch back, which can cause false positives
// for bad effect interactions.
EffectAnalyzer loopEffects(getPassOptions(), *getModule(), loop);
loopEffects.localsRead.clear();
loopEffects.localsWritten.clear();
// Note all the sets in each loop, and how many per index. Currently
// EffectAnalyzer can't do that, and we need it to know if we
// can move a set out of the loop (if there is another set
Expand Down Expand Up @@ -123,9 +126,8 @@ struct LoopInvariantCodeMotion
// take into account global state like interacting loads and
// stores.
bool unsafeToMove = effects.writesGlobalState() ||
effectsSoFar.invalidates(effects) ||
(effects.readsMutableGlobalState() &&
loopEffects.writesGlobalState());
effectsSoFar.orderedBefore(effects) ||
loopEffects.orderedBefore(effects);
Copy link
Copy Markdown
Member

@kripken kripken May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

effectsSoFar.orderedBefore(effects) makes sense to me. But for loopEffects.orderedBefore(effects), as you said they execute before, during, and after - why is it enough to only check one?

Copy link
Copy Markdown
Member

@kripken kripken May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(last question I promise!)

Copy link
Copy Markdown
Member Author

@tlively tlively May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we're only trying to move the execution of curr (whose effects are effects) before the loop, i.e. back past all the previous executions of the loop body. If we were trying to sink curr down past the loop, we would have to check the other direction.

Copy link
Copy Markdown
Member

@kripken kripken May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks. Yes, that's the right way to look at this.

// TODO: look into optimizing this with exceptions. for now, disallow
if (effects.throws() || loopEffects.throws()) {
unsafeToMove = true;
Expand Down
68 changes: 68 additions & 0 deletions test/lit/passes/licm-atomics.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
;; NOTE: Assertions have been generated by update_lit_checks.py and should not be edited.
;; RUN: foreach %s %t wasm-opt -all --licm -S -o - | filecheck %s

(module
;; CHECK: (type $struct (shared (struct (field (mut i32)))))
(type $struct (shared (struct (field (mut i32)))))

;; CHECK: (memory $mem 1 1 shared)
(memory $mem 1 1 shared)

;; Test 1: Allowed reordering (GC read moved before Wasm release store)
;; CHECK: (func $allowed (type $1) (param $x (ref $struct))
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (struct.get $struct 0
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (loop $loop
;; CHECK-NEXT: (i32.atomic.store acqrel
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: (i32.const 42)
;; CHECK-NEXT: )
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: (br $loop)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $allowed (param $x (ref $struct))
(loop $loop
;; X: release store (Wasm memory)
(i32.atomic.store acqrel (i32.const 0) (i32.const 42))
;; E: memory access (shared GC read)
(drop
(struct.get $struct 0 (local.get $x))
)
Copy link
Copy Markdown
Member

@kripken kripken May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In both of these tests, should we also test the the memory/GC instructions reordered? Here, with the struct.get before the i32 store?

Copy link
Copy Markdown
Member Author

@tlively tlively May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have very thorough tests of all the interactions considered by orderedBefore and orderedAfter, so here I think it's sufficient to have a single positive and single negative test showing that we got the order correct.

(br $loop)
)
)

;; Test 2: Disallowed reordering (GC read moved before Wasm acquire load)
;; CHECK: (func $disallowed (type $1) (param $x (ref $struct))
;; CHECK-NEXT: (loop $loop
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (i32.atomic.load acqrel
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (struct.get $struct 0
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (br $loop)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $disallowed (param $x (ref $struct))
(loop $loop
;; X: acquire load (Wasm memory)
(drop
(i32.atomic.load acqrel (i32.const 0))
)
;; E: memory access (shared GC read)
(drop
(struct.get $struct 0 (local.get $x))
)
(br $loop)
)
)
)
82 changes: 80 additions & 2 deletions test/lit/passes/licm.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@

;; CHECK: (type $0 (func (param i32)))

;; CHECK: (type $1 (func))
;; CHECK: (type $1 (func (param i32) (result i32)))

;; CHECK: (type $2 (func))

;; CHECK: (memory $0 10 20)

;; CHECK: (func $unreachable-get (type $1)
;; CHECK: (func $unreachable-get (type $2)
;; CHECK-NEXT: (local $x i32)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (local.get $x)
Expand Down Expand Up @@ -104,4 +106,80 @@
(br $loop)
)
)

;; CHECK: (func $bug-inversion (type $1) (param $z i32) (result i32)
;; CHECK-NEXT: (local $x i32)
;; CHECK-NEXT: (local $y i32)
;; CHECK-NEXT: (local.set $y
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $x
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: (loop $loop
;; CHECK-NEXT: (local.set $y
;; CHECK-NEXT: (i32.const 2)
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $x
;; CHECK-NEXT: (i32.add
;; CHECK-NEXT: (local.get $y)
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (br_if $loop
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: )
(func $bug-inversion (param $z i32) (result i32)
(local $x i32)
(local $y i32)
(local.set $y (i32.const 0))
(local.set $x (i32.const 0))
(loop $loop
(local.set $y (i32.const 2))
(local.set $x (i32.add (local.get $y) (i32.const 1)))
(br_if $loop (i32.const 0))
)
(local.get $x)
)
Copy link
Copy Markdown
Member

@kripken kripken May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is being tested by these? (and how does it relate to this PR?)

Copy link
Copy Markdown
Member Author

@tlively tlively May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I first tried to make simplifications based on your suggestion to clear the local effects outside the loop, I went overboard and produced something incorrect. But the test suite still passed. These are regression tests for that incorrect simplification.


;; CHECK: (func $bug-cross-statement-dependency (type $1) (param $z i32) (result i32)
;; CHECK-NEXT: (local $x i32)
;; CHECK-NEXT: (local $y i32)
;; CHECK-NEXT: (local.set $x
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $y
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: (loop $loop
;; CHECK-NEXT: (local.set $y
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $x
;; CHECK-NEXT: (i32.add
;; CHECK-NEXT: (local.get $z)
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (br_if $loop
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.get $y)
;; CHECK-NEXT: )
(func $bug-cross-statement-dependency (param $z i32) (result i32)
(local $x i32)
(local $y i32)
(local.set $x (i32.const 0))
(local.set $y (i32.const 0))
(loop $loop
(local.set $y (local.get $x))
(local.set $x (i32.add (local.get $z) (i32.const 1)))
(br_if $loop (i32.const 0))
)
(local.get $y)
)
)
Loading