-
Notifications
You must be signed in to change notification settings - Fork 28
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
Improve simpilfy to normalize polynomial before constant evaluation #186
Conversation
Codecov Report
@@ Coverage Diff @@
## master #186 +/- ##
==========================================
+ Coverage 79.94% 79.98% +0.04%
==========================================
Files 32 32
Lines 2857 2863 +6
==========================================
+ Hits 2284 2290 +6
Misses 573 573
Continue to review full report at Codecov.
|
src/exo/LoopIR_scheduling.py
Outdated
new_typ = T.Tensor(new_hi, False, s.type.basetype()) | ||
return [LoopIR.Alloc(s.name, new_typ, s.mem, s.eff, s.srcinfo)] | ||
|
||
return super().map_s(s) |
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.
Could you eliminate all of these different map_*
cases by simply checking at the top of map_e
whether or not an expression is of index
or size
type (i.e. is indexable
) and then returning the result of index_start
if it is?
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.
Yes, thanks!
return self.concat_map(e.op, lhs_map, rhs_map) | ||
else: | ||
assert False, ("index_start should only be called by"+ | ||
f" an indexing expression. e was {e}") |
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.
ReadConfig
can have indexable
type, and is unhandled.
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.
Thanks for catching!
src/exo/LoopIR_scheduling.py
Outdated
|
||
def normalize_e(self, e): | ||
if isinstance(e, LoopIR.Read): | ||
assert e.type.is_indexable() |
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.
Should this assertion be lifted up to the top scope of this method, applicable to all cases?
if op == '+': | ||
# if has same key: add value | ||
common = { key: (lhs[key]+rhs[key]) for key in lhs if key in rhs } | ||
return lhs | rhs | common |
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.
Are the semantics of union such that the right-hand side of the union is guaranteed to overwrite the left in the event of a key clash?
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.
Yes!
src/exo/LoopIR_scheduling.py
Outdated
if len(rhs) == 1 and self.C in rhs: | ||
return { key: lhs[key]*rhs[self.C] for key in lhs } | ||
else: | ||
assert self.C in lhs |
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.
len(lhs) == 1 and self.C in lhs
src/exo/LoopIR_scheduling.py
Outdated
new_e = LoopIR.BinOp('+', new_e, get_loopir(key, val), e.type, e.srcinfo) | ||
else: | ||
# sub | ||
new_e = LoopIR.BinOp('-', new_e, get_loopir(key, val), e.type, e.srcinfo) |
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.
Does this need to be -val
?
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.
Thanks for catching this!!
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.
Added a test for this
return LoopIR.BinOp('*', vconst, readkey, e.type, e.srcinfo) | ||
|
||
delete_zero = { key: n_map[key] for key in n_map if n_map[key] != 0 } | ||
new_e = LoopIR.Const(0, T.int, e.srcinfo) |
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.
Will this just generate gratuitous leading zeros everywhere, or does simplification take care of that for you?
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.
Simplification's constant folding path takes care of eliminating the leading zeros
codecov is being silly |
No description provided.