Make more Ifs unreachable #7094
Conversation
Previously the only Ifs that were typed unreachable were those in which both arms were unreachable and those in which the condition was unreachable that would have otherwise been typed none. This caused problems in IRBuilder because Ifs with unreachable conditions and value-returning arms would have concrete types, effectively hiding the unreachable condition from the logic for dropping concretely typed expressions preceding an unreachable expression when finishing a scope. Relax the conditions under which an If can be typed unreachable so that all Ifs with unreachable conditions or two unreachable arms are typed unreachable. Propagating unreachability more eagerly this way makes various optimizations of Ifs more powerful. It also requires new handling for unreachable Ifs with concretely typed arms in the Printer to ensure that printed wat remains valid.
| auto* ret = | ||
| builder.makeSequence(builder.makeDrop(curr->condition), curr->ifTrue); | ||
| builder.makeSequence(builder.makeDrop(curr->condition), ifTrue); |
There was a problem hiding this comment.
Reading this code and the updated test following it, I still don't understand the change. The code looks valid before and after, and perhaps it is slightly more optimal to add a drop here rather than to wait for another pass to do it, but is it worth the complexity? Is there another reason I haven't considered?
There was a problem hiding this comment.
We get a validation error without inserting the drop. Without this change, we parse this wat:
(func $foo (result i32)
i32.const 0
unreachable
if (result i32)
i32.const 1
else
i32.const 2
end
)
into this IR:
(func $foo (result i32)
(i32.const 0) ;; Validation error!
(if (result i32)
(unreachable)
(then
(i32.const 1)
)
(else
(i32.const 2)
)
)
)
This IR is invalid because the function body block has a non-final item with a concrete type. The IR should have contained a drop of that i32.const 0.
There was a problem hiding this comment.
Oh, this response wasn't specific to the CodeFolding logic, but it has a similar problem.
Down below we have ref->finalize(curr->type). If curr->type is Type::unreachable, then it was previously the case that curr->ifTrue must have been type none or unreachable, but now it can be any type. Finalizing a sequence block where the second item has type e.g. i32 as unreachable is not valid, so to fix it we have to drop that second item.
| auto* ifTrue = curr->ifTrue; | ||
| if (curr->type == Type::unreachable && curr->ifTrue->type.isConcrete()) { | ||
| ifTrue = builder.makeDrop(ifTrue); | ||
| } |
There was a problem hiding this comment.
Can use Builder's method dropIfConcretelyTyped
|
Thanks! I'm still finding fuzz bugs with this, so I'll ping you explicitly for final review once the fixes are in. |
|
@kripken, this should be good to go now. PTAL at the changes since your last review. I'll continue fuzzing overnight before landing this, though. |
Code folding does not support folding tails that produce concrete values, but it previously did not check for this condition when deciding whether to attempt to code fold ifs. As a result, code folding would proceed on ifs with concretely typed arms. The incorrect block types produced by the folding logic as a result of the violated assumption that the folded tails would never produce concrete values were papered over by later refinalization, so this never caused problems. However, an upcoming change (#7094) that relaxes the typing of ifs to allow them to be unreachable whenever their conditions are unreachable makes it possible for the violated assumptions in code folding to cause problems that are not fixed by refinalization. Fix code folding to disallow folding of concretely typed if arms and add a test that would fail once #7094 lands without this fix.
Previously the only Ifs that were typed unreachable were those in which
both arms were unreachable and those in which the condition was
unreachable that would have otherwise been typed none. This caused
problems in IRBuilder because Ifs with unreachable conditions and
value-returning arms would have concrete types, effectively hiding the
unreachable condition from the logic for dropping concretely typed
expressions preceding an unreachable expression when finishing a scope.
Relax the conditions under which an If can be typed unreachable so that
all Ifs with unreachable conditions or two unreachable arms are typed
unreachable. Propagating unreachability more eagerly this way makes
various optimizations of Ifs more powerful. It also requires new
handling for unreachable Ifs with concretely typed arms in the Printer
to ensure that printed wat remains valid.