Handle loops in analysis of assigned vars #925
Conversation
Codecov Report
@@ Coverage Diff @@
## main #925 +/- ##
==========================================
- Coverage 76.91% 76.85% -0.07%
==========================================
Files 112 112
Lines 13669 13723 +54
Branches 1397 1405 +8
==========================================
+ Hits 10514 10547 +33
- Misses 2804 2826 +22
+ Partials 351 350 -1
|
onnxscript/analysis.py
Outdated
| if isinstance(stmt, ast.Break): | ||
| return set() | ||
| if ast_utils.is_print_call(stmt): | ||
| return set() | ||
| if isinstance(stmt, list): | ||
| return assigned_in_block(stmt) |
There was a problem hiding this comment.
Should we test this too?
There was a problem hiding this comment.
It is tested by every test for this function, since parse_tree.body is a list of statements. In fact, this extension exists primarily for enabling the unit tests.
There was a problem hiding this comment.
hmm strange.
coverage shows only this line is not tested
There was a problem hiding this comment.
Let me print some things here to see
There was a problem hiding this comment.
Duplicate of line 77?
There was a problem hiding this comment.
Good catch, thanks!
| live_defs = list( | ||
| stmt.live_out.intersection(analysis.assigned_vars(stmt, self.message)) | ||
| ) | ||
| else: | ||
| live_defs = list(analysis.defs(stmt)) | ||
| live_defs = list(analysis.assigned_vars(stmt, self.message)) |
There was a problem hiding this comment.
Would be good to explain this logic with a comment?
There was a problem hiding this comment.
Good question. I didn't add it, and it seems questionable to me. I need to check it out, I suspect it was added in some case where the liveness analysis had not run for some reason. but the fix should probably be something else.
The analysis function "assigned_vars" was not handling loops. Fix this.
Should fix issue #879
See also PR #909