Repository navigation
Conversation
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.
| // 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. |
There was a problem hiding this comment.
What do we think about this solution? The alternative would be to inhibit these optimizations in the presence of pops.
There was a problem hiding this comment.
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[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
| pop = *popPtr; | ||
| isPopNested = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we separate this fix from adding calls to the two passes?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
| // 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. |
There was a problem hiding this comment.
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.
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. |
This branch has not been deployed
3 participants
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.