Perform CSE across block boundaries.#10105
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
facebook-github-bot
left a comment
There was a problem hiding this comment.
resistor has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
There was a problem hiding this comment.
resistor is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
facebook-github-bot
left a comment
There was a problem hiding this comment.
resistor has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@apaszke ping |
apaszke
left a comment
There was a problem hiding this comment.
Actually when does it happen that CSE replaces a node with a different output type, even though its kind and all types of inputs are the same! We shouldn't really have nodes overloaded on output types
|
@apaszke I did not observe any differences in test results switching from kind equality + input equality to operator==. That said, even if the difference does not inherently matter in CSE, I'd prefer to use the semantically correct check to make this easier to maintain in case CSE or our operator changes in the future in a way that would break the clever way. |
|
@resistor this is because most of our types are singletons, and so their pointers will compare equal iff they compare equal. However, this is not always true (e.g. TensorType has many distinct instances), and we should use the reference-level equality in place of the pointer-level one. Do you remember which test has triggered failure because of missing equality checks? I'm not 100% sure if they're needed. |
|
@apaszke |
|
@apaszke What should be the next step for this patch? |
zdevito
left a comment
There was a problem hiding this comment.
I think all the changes were addressed and this is ready to land.
@zdevito