[JIT] API for finding a common ancestor block for a pair of nodes #28864
Conversation
[ghstack-poisoned]
facebook-github-bot
added
the
oncall: jit
…f nodes" Differential Revision: [D18219786](https://our.internmc.facebook.com/intern/diff/D18219786) [ghstack-poisoned]
suo
left a comment
suo
left a comment
There was a problem hiding this comment.
Can we add some tests for this functionality? The IR Parser should make it not terrible
| } | ||
|
|
||
| private: | ||
| Block* findCommonAncestorBlockWith(Node* n); |
There was a problem hiding this comment.
There are also a number of places where we do stuff like this, it could be nice to clean them up by using this API. Topological moves come to mind
There was a problem hiding this comment.
This function could be refactored to using this API:
Line 1176 in 4bfe2f0
There was a problem hiding this comment.
Hmm, I tried using this API for that isBeforeOrAfter but isBeforeOrAfter requires having pointers to the ancestors Nodes that reside in the common ancestor block
There was a problem hiding this comment.
Sorry i meant to say isBeforeOrAfter could be refactored using this function. but actually i take it back
…f nodes" Differential Revision: [D18219786](https://our.internmc.facebook.com/intern/diff/D18219786) [ghstack-poisoned]
…f nodes" Differential Revision: [D18219786](https://our.internmc.facebook.com/intern/diff/D18219786) [ghstack-poisoned]
…f nodes" Differential Revision: [D18219786](https://our.internmc.facebook.com/intern/diff/D18219786) [ghstack-poisoned]
eellison
left a comment
eellison
left a comment
There was a problem hiding this comment.
Looks good, a few comments / nits
| ASSERT_EQ(name_to_value.size(), 4); | ||
|
|
||
| /* clang-format off */ | ||
| int ref_blocks_from_graph[4][4] = { |
There was a problem hiding this comment.
Encoding expected results like this works, another alternative is to include your previous algorithm in this test file and check against that.
| } | ||
|
|
||
| private: | ||
| Block* findCommonAncestorBlockWith(Node* n); |
There was a problem hiding this comment.
This function could be refactored to using this API:
Line 1176 in 4bfe2f0
…f nodes" Differential Revision: [D18219786](https://our.internmc.facebook.com/intern/diff/D18219786) [ghstack-poisoned]
|
@jamesr66a merged this pull request in 0f4b226. |
Stack from ghstack:
Differential Revision: D18219786