Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request simplifies the getTempForBreakTarget helper in the Flatten pass by removing its redundant Type parameter and instead resolving the target block's type internally via findBreakTarget. It also adds a test case to verify correct handling of subtyping with br_table. The review feedback suggests optimizing the type comparison check in Flatten.cpp by querying the local variable's type directly from the function instead of performing a redundant AST traversal with findBreakTarget.
| // second 'flowing out' local in case two block's types are | ||
| // different. | ||
| if (type != blockType) { | ||
| if (type != findBreakTarget(br->name)->type) { |
There was a problem hiding this comment.
Instead of calling findBreakTarget(br->name)->type again, which performs a redundant AST traversal up the parent stack, you can query the type of the local variable temp directly from the function using getFunction()->getLocalType(temp). Since temp was just retrieved or allocated using getTempForBreakTarget(br->name), its local type is guaranteed to match the target block's type.
| if (type != findBreakTarget(br->name)->type) { | |
| if (type != getFunction()->getLocalType(temp)) { |
There was a problem hiding this comment.
That is true, but more complicated (harder to refactor, in particular), and would require a comment.
|
Looks like a dup of #9312 |
We assumed its value's type was identical to the type of the
thing we branch to, but the target may be less refined. Just
make
getTempForBreakTargetalways compute the rightthing.