-
Notifications
You must be signed in to change notification settings - Fork 229
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
compiler: Patch CSE in presence of conditionals #2392
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2392 +/- ##
=======================================
Coverage 86.70% 86.70%
=======================================
Files 234 234
Lines 44409 44424 +15
Branches 8220 8219 -1
=======================================
+ Hits 38503 38518 +15
Misses 5185 5185
Partials 721 721 ☔ View full report in Codecov by Sentry. |
pe, changed = _uxreplace(pe, {k: v}) | ||
if changed and v not in scheduled: | ||
updated.append(pe.func(v, k, operation=None)) | ||
scheduled.append(v) | ||
updated.append(pe) | ||
processed = updated |
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.
Why not just use updated
henceforth, rather than using processed
?
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.
wdym ? we keep on looping and processed
needs to be updated? what change are you proposing ?
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.
Ignore me 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.
Missed an indent
@@ -168,6 +168,29 @@ def test_cse_temp_order(): | |||
assert type(args[2]) is CTemp | |||
|
|||
|
|||
def test_cse_w_conditionals(): | |||
grid = Grid(shape=(10, 10, 10)) | |||
x, _, _ = grid.dimensions |
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.
Nitpick: x = grid.dimensions[0]
would be tidier imo
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.
tidier but (nitpicking) the one above performs an implicit assertion
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.
Fairs
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.
Comments responded, looks good
3f80ae9
to
90ad3e4
Compare
No description provided.