Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Liveness for BailOut graphs #21615

Closed
wants to merge 2 commits into from
Closed

Liveness for BailOut graphs #21615

wants to merge 2 commits into from

Conversation

Krovatkin
Copy link
Contributor

No description provided.

@Krovatkin
liveness
5233c03 
tidy fixes, clang-format
@Krovatkin Krovatkin requested a review from zdevito June 10, 2019 21:56
@pytorchbot pytorchbot added caffe2 oncall: jit Add this issue/PR to JIT oncall triage queue module: build Build system issues module: cpp Related to C++ API module: internals Related to internal abstractions in c10 and ATen labels Jun 10, 2019
zdevito
zdevito approved these changes Jun 11, 2019
View reviewed changes
Copy link
Contributor

@zdevito zdevito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, minor comments.

torch/csrc/jit/passes/liveness.cpp
}

std::shared_ptr<Graph> graph_;
std::map<Node*, SparseBitVector> liveness_sets_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why map?

torch/csrc/jit/passes/liveness.h

using ::c10::ProfiledTensorTypePtr;
using SparseBitVector = ::c10::SparseBitVector<256>;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment descibing the form of the output? Is that live in or live out of a node?

torch/csrc/jit/passes/liveness.cpp Outdated
liveness |= block_outputs;

SparseBitVector defs;
for (auto it = b->nodes().rbegin(); it != b->nodes().rend(); it++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for (Node* n : b->nodes().reverse())

torch/csrc/jit/passes/liveness.cpp Outdated
}

liveness |= toSparseBitVector(it->inputs());
liveness -= toSparseBitVector(it->outputs());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to go at the beginning of the loop right? Otherwise the outputs of an if statement will appear live in the body of the if statement. This line is also repeated twice for a loop which seems wrong.

torch/csrc/jit/passes/liveness.cpp Outdated
: graph_(std::move(graph)) {}

std::map<Node*, std::vector<Value*>> run() {
auto function_outputs = toSparseBitVector(graph_->block()->outputs());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is redundant, right? the first thing visiting a block does is union the outputs into the liveness.

facebook-github-bot
facebook-github-bot reviewed Jun 12, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Krovatkin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@Krovatkin merged this pull request in 8dd6706.

@jamesr66a
Copy link
Collaborator

Pretty sure this broke master:

Jun 13 01:14:27 ======================================================================
Jun 13 01:14:27 ERROR: test_cpp (__main__.TestJit)
Jun 13 01:14:27 ----------------------------------------------------------------------
Jun 13 01:14:27 Traceback (most recent call last):
Jun 13 01:14:27   File "/var/lib/jenkins/workspace/test/common_utils.py", line 149, in wrapper
Jun 13 01:14:27     fn(*args, **kwargs)
Jun 13 01:14:27   File "test_jit.py", line 1618, in test_cpp
Jun 13 01:14:27     torch._C._jit_run_cpp_tests(run_cuda=False)
Jun 13 01:14:27 RuntimeError: (actual_loop_liveness) == (expected_for) INTERNAL ASSERT FAILED at /var/lib/jenkins/workspace/test/cpp/jit/test_misc.h:995, please report a bug to PyTorch.  (testLivenessFor at /var/lib/jenkins/workspace/test/cpp/jit/test_misc.h:995)
Jun 13 01:14:27 frame #0: c10::Error::Error(c10::SourceLocation, std::string const&) + 0x47 (0x7f2bdbd2c607 in /opt/python/2.7.9/lib/python2.7/site-packages/torch/lib/libc10.so)
Jun 13 01:14:27 frame #1: <unknown function> + 0x295fdff (0x7f2bdeaa5dff in /opt/python/2.7.9/lib/python2.7/site-packages/torch/lib/libcaffe2.so)
Jun 13 01:14:27 frame #2: torch::jit::runJITCPPTests(bool) + 0xcd (0x7f2bdeaf2b5d in /opt/python/2.7.9/lib/python2.7/site-packages/torch/lib/libcaffe2.so)
Jun 13 01:14:27 frame #3: <unknown function> + 0x393f81 (0x7f2be0706f81 in /opt/python/2.7.9/lib/python2.7/site-packages/torch/lib/libtorch_python.so)
Jun 13 01:14:27 frame #4: <unknown function> + 0x1960d6 (0x7f2be05090d6 in /opt/python/2.7.9/lib/python2.7/site-packages/torch/lib/libtorch_python.so)
Jun 13 01:14:27 <omitting python frames>
Jun 13 01:14:27 frame #54: __libc_start_main + 0xf5 (0x7f2bf0124f45 in /lib/x86_64-linux-gnu/libc.so.6)
Jun 13 01:14:27 frame #55: /opt/python/2.7.9/bin/python() [0x40070e]

@Krovatkin
Copy link
Contributor Author

@jamesr66a i'll disable the tests, i didn't expect them to be so unstable 😢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@zdevito zdevito zdevito approved these changes

Assignees
No one assigned
Labels
caffe2 Merged module: build Build system issues module: cpp Related to C++ API module: internals Related to internal abstractions in c10 and ATen oncall: jit Add this issue/PR to JIT oncall triage queue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Krovatkin @facebook-github-bot @jamesr66a @zdevito @pytorchbot @mruberry