[CFP][GSI] Fix catch pops after optimizing reads by tlively · Pull Request #9289 · WebAssembly/binaryen

tlively · 2026-10-20T20:34:43Z

When ConstantFieldPropagation or GlobalStructInference optimizes a struct.get or ref.get_desc, it either wraps the reference operand in a Block or places it into the condition (third child) of a Select. Any pop instruction inside the reference from a legacy exception-handling catch body then becomes either nested inside a block or displaced out of the catch body's first-descendant chain. Update EHUtils::handleBlockNestedPop to also find and lift pops that have been moved out of the first-descendant chain, and call EHUtils::handleBlockNestedPops in both passes when reads are optimized.

tlively · 2026-10-20T20:35:21Z

+      // The pop may have been moved out of the first-descendant line, e.g.
+      // into the condition of a select. Find it and fix it up just like a
+      // block-nested pop.


What do we think about this solution? The alternative would be to inhibit these optimizations in the presence of pops.

If we are gonna ignore getFirstPop's "no", we probably should remove it and replace it with this? There's no point in calling and keeping getFirstPop at this point.

gemini-code-assist

Code Review

This pull request introduces a helper function findPopPtr in eh-utils.cpp to locate nested Pop expressions and fix them up when they are moved out of the first-descendant line (e.g., into select conditions or blocks). This fix-up logic is integrated into the ConstantFieldPropagation and GlobalStructInference passes, with accompanying tests. A critical issue was identified in the new findPopPtr function where curr can be nullptr for optional AST children, which would lead to a segmentation fault when calling curr->is<Pop>(). A null check should be added to prevent this.

kripken · 2026-10-20T22:13:55Z

+        pop = *popPtr;
+        isPopNested = true;
+      }
+    }


Can we separate this fix from adding calls to the two passes?

The reason this fix goes along with these two passes is that these passes are the only ones (that we know about) that can move the pop out of that first-child position. We could of course split it up in principle, but I think it makes sense to keep them together.

Ok, then let me try to understand this change more. How does getFirstPop differ from findPopPtr? The new function isn't documented and I'm not sure exactly what it is doing yet.

getFirstPop only looks at the leftmost leaf in the expression tree, i.e. where pop is supposed to be found. findPopPtr looks for a pop in the entire expression tree, except inside nested trys.

Thanks, though now I'm a little fuzzy on why we need both - the motivation/background is not clear to me?

aheejin

I think the reason I only allowed pops in first-descendant lines in #4348 was because otherwise the code was invalid, and all optimizations that needed the fixup so far were only nesting pops into a block and nothing more. (@kripken Is this right? You are who added almost all fixup code in various passes. Does this reflect your experience, or you saw cases where pops were displaced out of first-descendant lines but you just gave up optimizing them?)

Anyway, if we are to allow pops to be hoisted from anywhere within a catch block, I think we should just replace it with the existing first-descendant searching (because we are ignoring its result anyway). I'm also wondering, should we go into loops, where pops can run multiple times, and if's true/false bodies, where pops run conditionally? Or should we just avoid going into nested catches?

aheejin · 2026-10-21T00:24:09Z

+      // The pop may have been moved out of the first-descendant line, e.g.
+      // into the condition of a select. Find it and fix it up just like a
+      // block-nested pop.


If we are gonna ignore getFirstPop's "no", we probably should remove it and replace it with this? There's no point in calling and keeping getFirstPop at this point.

tlively · 2026-10-21T00:35:34Z

I'm also wondering, should we go into loops, where pops can run multiple times, and if's true/false bodies, where pops run conditionally? Or should we just avoid going into nested catches?

I think it would be harmless to search in ifs and loops. It's hard to imagine a case where an optimization would be able to move a pop into an if or a loop, and maybe that never makes sense. But if it did make sense, then we would definitely need to fix it up. I don't think there's much downside to searching in ifs or loops, either. I doubt it makes much of a performance difference.

kripken · 2026-10-21T17:42:19Z

@aheejin

and all optimizations that needed the fixup so far were only nesting pops into a block and nothing more. (@kripken Is this right? You are who added almost all fixup code in various passes. Does this reflect your experience,

Yes, that is all I have seen. Actually I was just thinking that since that is the common case, maybe there is a simpler solution to all this. Yesterday I started Gemini on an attempt to allow multiple unnamed blocks in validation - will report back when it finishes.

kripken · 2026-10-21T23:14:09Z

Ok, #9292 is up for feedback. If that direction looks good maybe we don't need these CFP/GSI changes?

tlively · 2026-10-21T23:36:50Z

We still need these changes because pops can only ever validate in the leftmost leaf position, and the problem here is that CFP and GSI do not preserve this.

kripken

Makes sense, happy to review this.

kripken · 2026-10-21T23:39:50Z

  return pop != nullptr && !isPopNested;
 }

+static Expression** findPopPtr(Expression*& expr) {


Maybe add a comment on this function?

tlively · 2026-10-22T00:39:03Z

Ok, I've pushed a new implementation that 1) replaces getFirstPop entirely, and 2) uses a PostWalker with a custom scan to be as efficient as possible. It also includes the simplification around unnamed blocks from #9292, so that will need to land before the tests pass here.

tlively · 2026-10-22T00:39:43Z


    // If we changed anything, we need to update parent types as types may have
-    // changed.
+    // changed, and fix up any pops that we enclosed in a block.


Comment needs updating after #9292.

tlively · 2026-10-22T00:40:03Z

          // to synchronize with in the first place.
          ref = builder.makeSequence(
            builder.makeDrop(builder.makeRefAs(RefAsNonNull, ref)), get);
+          needEHFixups = true;


Unnecessary after #9292.

tlively · 2026-10-22T00:40:07Z

          replaceCurrent(builder.makeSequence(
            builder.makeDrop(builder.makeRefAs(RefAsNonNull, ref)),
            getReadValue(values[0], fieldIndex, field, curr)));
+          needEHFixups = true;


Unnecessary after #9292.

tlively · 2026-10-22T00:42:43Z

+    ;; Optimizing this struct.get wraps its reference in a block, requiring the
+    ;; enclosed pop to be fixed up.


Unnecessary after #9292. We can probably just remove the test, or alternatively keep it as a test showing that we need no fixup.

tlively · 2026-10-22T00:43:14Z

+    ;; Optimizing this struct.get wraps its reference in a block, requiring the
+    ;; enclosed pop to be fixed up.


Unnecessary after #9292.

tlively requested a review from a team as a code owner October 20, 2026 20:34

tlively requested review from aheejin, kripken and stevenfontanella and removed request for a team October 20, 2026 20:34

tlively commented Oct 20, 2026

View reviewed changes

gemini-code-assist Bot reviewed Oct 20, 2026

View reviewed changes

Comment thread src/ir/eh-utils.cpp Outdated

kripken reviewed Oct 20, 2026

View reviewed changes

aheejin reviewed Oct 21, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into cfp-block-nested-pops

2b05e0a

tlively mentioned this pull request Oct 21, 2026

Allow pops to validate inside unnamed blocks, removing almost all handleBlockNestedPop() calls #9292

Draft

kripken reviewed Oct 21, 2026

View reviewed changes

replace getFirstPop, use PostWalker, depend on #9292

6f9cac0

tlively commented Oct 22, 2026

View reviewed changes

		;; Optimizing this struct.get wraps its reference in a block, requiring the
		;; enclosed pop to be fixed up.

Conversation

tlively commented Oct 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aheejin Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aheejin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aheejin Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlively commented Oct 21, 2026

Uh oh!

kripken commented Oct 21, 2026

Uh oh!

kripken commented Oct 21, 2026

Uh oh!

tlively commented Oct 21, 2026

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlively commented Oct 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aheejin Oct 21, 2026 •

edited

Loading

aheejin left a comment •

edited

Loading

aheejin Oct 21, 2026 •

edited

Loading