loop_invariant_elimination: require the body to return the loop var itself - #2804
Open
LeSingh1 wants to merge 1 commit into
Open
loop_invariant_elimination: require the body to return the loop var itself#2804LeSingh1 wants to merge 1 commit into
LeSingh1 wants to merge 1 commit into
Conversation
…tself The pass flagged a loop var as invariant when the body's output for that var was "a var from outside of the block", which it tested with Block.is_var_visible_in_block. That is true of any var in an enclosing scope, not only of the loop var the body was seeded with. When the body returns a different outer var the loop var is not invariant at all: it is the seed on the first iteration and that other var from the second on. The pass then dropped the loop var and rewrote the while_loop output to the seed, so the model silently returned the wrong tensor. With loop_vars=(a, b), a body returning (add(a, 1), c) and three iterations, the output that should be c came back as b. Test the body output against while_op.loop_vars[i] instead. The existing "block outputs var from outside of the block" case still qualifies, because there the outer var is the loop var. Such a body output also has to be given a definition inside the body, since a block output has to be produced in its own block; leaving a raw enclosing-scope var there produces a model Core ML cannot load. Re-emit it with an identity in the body block.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
loop_invariant_eliminationdecides that loop variis invariant if either of these holds:The first test is sound. The second is not:
Block.is_var_visible_in_blockis true of any var in an enclosing scope — a function input, an_internal_var, or the output of any op before thewhile_loop. It says nothing about whether that var iswhile_op.loop_vars[i].When the body returns a different outer var, the loop var is not invariant. It is the seed on the first iteration and the other var from the second on. The pass nevertheless drops the loop var and rewrites the
while_loopoutput to the seed.Repro
The loop runs three times, so output 1 should be
c. Converting onmainand predicting witha = 0,b = 10,c = 20:i.e.
b. The pass reduced the loop to one loop var and emitted%while_loop_0_1 = identity(x=%b). Nothing warns;op.enclosing_block.validate()at the end of the pass only checks that the program is structurally well formed.The existing
test_loop_invariant_elimination2is titled "Invariant pattern: Block outputs var from outside of the block", and there the body does return an outer var — but it returnsb, which isloop_vars[1]. That coincidence is the precondition the code never checks.Fix
Test the body output against
while_op.loop_vars[i]instead of against visibility. The existing "outputs a var from outside of the block" case still qualifies, because there the outer var is the loop var.loop_vars[i]is an operand of thewhile_loop, so it is visible in the enclosing block by construction and the visibility query adds nothing.The pass also has to give such a body output a definition inside the body, because a block output has to be produced in its own block. Left alone, the program reaches the backend with
-> (%add_0, %c)and Core ML segfaults while loading the compiled model (inMLModel._get_proxy_and_spec). Re-emitting the value with anidentityinside the body block converts and runs correctly, returning[[20.0, 20.0]].Also dropped a stale comment:
blockhere isblocks[1], the body, whose outputs are the loop vars, not the cond var.Testing
Two new tests in
TestLoopInvariantElimination, both failing onmain:test_loop_invariant_elimination_other_outer_varruns the pass and checks the loop var survives and the body output is now defined in the body.test_loop_invariant_elimination_other_outer_var_predictionruns the same program throughrun_compare_builderand checks output 1 isc. Skipped on theneuralnetworkbackend, which has nowhile_loop.The two existing tests are unchanged and still pass, as does the rest of
test_cleanup_passes.py.