compiler: Turn aliases' choose() into an instance method #2839
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2839 +/- ##
==========================================
- Coverage 78.94% 78.94% -0.01%
==========================================
Files 248 248
Lines 50877 50877
Branches 4394 4394
==========================================
- Hits 40167 40163 -4
- Misses 9912 9915 +3
- Partials 798 799 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
EdCaunt
left a comment
EdCaunt
left a comment
There was a problem hiding this comment.
Approving as this pretty much just moves stuff around, even if there are changes that I would personally consider
| # Project the candidate aliases into `exprs` to derive the final | ||
| # working set | ||
| mapper = {k: v for k, v in mapper.items() | ||
| if v.free_symbols & set(aliases.aliaseds)} |
There was a problem hiding this comment.
set(aliases.aliaseds) is known before creating this mapper and will be the same for every iteration of the list comprehension. I would maybe pull it out of the list comprehension?
| # `score > M` => optimized | ||
| # `m <= score <= M` => maybe optimized, depends on heuristics | ||
| m = self.opt_mingain | ||
| M = self.opt_mingain*3 |
There was a problem hiding this comment.
Why 3? Is it just a magic number?
| owset = wset(templated) | ||
|
|
||
| # Filter off the aliases with a weak flop-reduction / working-set tradeoff | ||
| key = lambda a: \ |
There was a problem hiding this comment.
Maybe this wants to be an actual function rather than a lambda - it's pretty unreadable
There was a problem hiding this comment.
Ah, just saw that all this is just lifted from elsewhere. I think the point still stands even if it's not so pressing
|
all your points do make sense as explained offline, we should revamp this module at some point in the future |
No description provided.