Conversation
wujingyue
changed the title
wujingyue
force-pushed
the
wjy/nonpack
branch
from
f8c6ace to
ef937d6
Compare
|
!build |
wujingyue
force-pushed
the
wjy/nonpack
branch
2 times, most recently
from
ce55952 to
61ef979
Compare
|
!build |
jacobhinkle
left a comment
jacobhinkle
left a comment
There was a problem hiding this comment.
Some initial comments.
| for (Expr* transform : DependencyCheck::getAllExprsBetween( | ||
| {out_root.begin(), out_root.end()}, | ||
| {out_rfactor.begin(), out_rfactor.end()})) { |
There was a problem hiding this comment.
You can use StmtSort::getExprsBetween for this.
There was a problem hiding this comment.
What's the benefit of that? I also saw Dependencies::getAllExprs. I'm confused when to use which...
There was a problem hiding this comment.
@jacobhinkle I merged my PR but would still appreciate your thought on this. I found at least three functions doing the similar work :) I'm not sure when to use which.
There was a problem hiding this comment.
Great question. It seems to me like DependencyCheck and StmtSort are redundant; they are used roughly equally often and appear to provide the same functionality, with DependencyCheck maybe slightly more complicated. DependencyCheck has been around since the beginning of the project, while StmtSort evolved from ExprSort (csarofeen/pytorch#1376). ExprSort was introduced in https://github.com/pytorch/pytorch/pull/45218/files#diff-49787cacb3e793473c24bc615113fb2c782086cbe552c63ac75ffede45f417a1R240-R254. I have been mostly using StmtSort which has options for controlling whether we traverse members and attributes. We should make clear in the comments when to use each. CC @naoyam @jjsjann123
There was a problem hiding this comment.
IIUC, Dependencies has been used from a very beginning of the project, and later IterVisitor::traverseBetween and StmtSort were added. I believe StmtSort basically reproduces the same functionality as Dependencies but more concisely using IterVisitor::traverseBetween. I think it should be valid to replace all DependencyCheck::getAllExprs/ValsBetween with StmtSort::getExprs/Vals, but should be confirmed by @csarofeen
jjsjann123
left a comment
jjsjann123
left a comment
There was a problem hiding this comment.
minor comments on some c++ nitpick. lgtm overall~~~
jjsjann123
left a comment
jjsjann123
left a comment
There was a problem hiding this comment.
Only realized it when I happen to see the hasAllocation check on LoadStoreOp
wujingyue
force-pushed
the
wjy/nonpack
branch
from
61ef979 to
9e8cdd2
Compare
|
!build |
wujingyue
force-pushed
the
wjy/nonpack
branch
from
4dc97e8 to
3e8c98f
Compare
|
!build |
My #1295 changed some tests in the wrong way so this bug has been hidden since then.
5 participants
Partially taken from #1281 to keep that PR small.