-
Notifications
You must be signed in to change notification settings - Fork 734
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.
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
Make br_if with a value's type match the spec #6390
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't thought about the fact that calculating precise types requires a fixed point computation after this fix. Maybe we should press a little harder on this issue after all.
// After that walk we may have br_ifs in need of refinalization. Update them | ||
// and refinalize again, as they may enable further improvements. This is in | ||
// theory very slow, but in practice one or two cycles suffices and we can't | ||
// try to be frugal here as must propagate all the possible improvements, or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// try to be frugal here as must propagate all the possible improvements, or | |
// try to be frugal here as we must propagate all the possible improvements, or |
if (auto* ret = curr->dynCast<Return>()) { | ||
value = ret->value; | ||
} else { | ||
value = curr->cast<Break>()->value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can never be a Switch
or any other branching instruction?
// to GC). This also calls finalize as needed, so that after calling it the | ||
// br_if type is fully updated. | ||
using BlockTypeMap = std::unordered_map<Name, Type>; | ||
static void updateBrIfType(Break* br, const BlockTypeMap& blockTypeMap) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this to avoid having to do a full ReFinalize
to fix the br_if
types? If so, is it really worth the extra complexity?
|
||
// Note types of blocks. This parallels the logic in the parent class, | ||
// but we do not want to just reuse the data structure there: things may | ||
// have changed since then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed since when?
if (isTuple()) { | ||
for (auto t : *this) { | ||
if (t.isRef()) { | ||
return true; | ||
} | ||
} | ||
return false; | ||
} | ||
return isRef(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can iterate through non-tuple types as well, and the iterator will just dereference to the type itself.
if (isTuple()) { | |
for (auto t : *this) { | |
if (t.isRef()) { | |
return true; | |
} | |
} | |
return false; | |
} | |
return isRef(); | |
} | |
for (auto t : *this) { | |
if (t.isRef()) { | |
return true; | |
} | |
} | |
return false; | |
} |
// There is no block by that name, so this must target the function scope. | ||
// We could also validate that it is in fact the function scope, but the | ||
// main validator does that anyhow; all we need here is to generate valid | ||
// IR if it is valid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although it has been suggested as an extension to the text format, there is not currently a way to use a symbolic name to have a branch target the function scope, so I don't think the old parser needs to (or should be able to) support this case.
@@ -781,6 +797,11 @@ void FunctionValidator::visitLoop(Loop* curr) { | |||
"breaks to a loop cannot pass a value"); | |||
} | |||
breakTypes.erase(iter); | |||
|
|||
// Validate there are no br_ifs with values targeting us. | |||
shouldBeTrue(!labelBrIfs.count(curr->name), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not covered by the breakTypes
check above?
I also just realized that this refinalization algorithm does not find the smallest fixed point of the types (i.e. the most refined possible Consider this program:
If I'm not mistaken, The fix would be to do the first |
Good point about this not necessarily reaching the optimal point. And, interesting idea to solve it. Thinking about that, perhaps another option is to first walk with the old behavior, and then to add casts as necessary afterwards, without any further iterations? That is, any |
Oh yeah, adding casts seems fine, too. It gets back all of the type precision at the cost of having to execute the casts at runtime. How much do you think that would simplify this PR beyond ReFinalize itself? Since following the spec rules has more downsides than I had anticipated, I also started pushing more to get them changed: WebAssembly/gc#516 (comment). |
Hmm, good question. I think adding the casts would remove a little bit of complexity (while not adding much extra tracking logic). But that would still leave 99% of the complexity in this PR. Good idea to discuss spec options. |
If we are able to change the spec, then we'll have to fix our handling of |
Yes, updating Reading that and WebAssembly/gc#201 again, it's hard not to see how we seem to keep returning to this topic again and again... 😄 🔁 |
Closing in favor of #6510 |
Previously
br_if
with a value had the same type as the value, but the specsays it should be the type of the block it targets, which might be less refined.
Context:
WebAssembly/gc#516 (comment)
This is unfortunate for Binaryen as the value is right there in our IR, while
looking up the block's type is more effort. I measure a 1.5% slowdown in
building a J2Wasm testcase with this. There do not seem to be downsides
to the output, but that is just because the value output of
br_if
is practicallynever used outside of fuzz testcases and exploits.
Summary of the necessary changes for this:
br_if
s - we can nolonger just do a simple forward pass here, as block changes go "backwards"
to
br_if
s. In practice I think that should be rare (on J2Wasm I see only a3 times where an extra iteration is needed when running
-O3
).br_if
types in the few passes that modifybr_if
values or addvalues or conditions to
br
s, including Heap2Local, RemoveUnusedBrs,SimplifyLocals.
used to do that, and they generally ended up refined later anyhow, but now
this would be a danger (they would unrefine the
br_if
, which might break).br_if
types during construction.Type.containsRef()
helper.