[nnc] Updated IR cloning to create clones of expressions in addition to statements #62833

navahgar · 2021-08-05T17:13:46Z

Stack from ghstack:

-> [nnc] Updated IR cloning to create clones of expressions in addition to statements #62833

Differential Revision: D30135980

…to statements [ghstack-poisoned]

facebook-github-bot · 2021-08-05T17:13:52Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/62833
📄 Preview docs built from this PR

💊 CI failures summary and remediations

As of commit 28dd109 (more details on the Dr. CI page):

1/2 failures introduced in this PR
1/2 tentatively recognized as flaky ❄️
- Click here to rerun these jobs

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

Lint / clang-tidy (1/1)

Step: "Check for warnings" (full log | diagnosis details | 🔁 rerun)

2021-08-06T23:21:06.8697646Z /__w/pytorch/pytor...move [modernize-pass-by-value,-warnings-as-errors]

2021-08-06T23:21:06.6416442Z �[36;1mcd "${GITHUB_WORKSPACE}"�[0m
2021-08-06T23:21:06.6416766Z �[36;1mset -eu�[0m
2021-08-06T23:21:06.6417191Z �[36;1mcat "${GITHUB_WORKSPACE}"/clang-tidy-output.txt�[0m
2021-08-06T23:21:06.6417864Z �[36;1mif grep -Fq "Warnings detected!" "${GITHUB_WORKSPACE}"/clang-tidy-output.txt; then�[0m
2021-08-06T23:21:06.6418491Z �[36;1m  echo 'Please fix the above clang-tidy warnings.'�[0m
2021-08-06T23:21:06.6418892Z �[36;1m  false�[0m
2021-08-06T23:21:06.6419141Z �[36;1mfi�[0m
2021-08-06T23:21:06.6419577Z shell: sh -e {0}
2021-08-06T23:21:06.6419862Z ##[endgroup]
2021-08-06T23:21:06.8695826Z Processing 1 clang-tidy jobs
2021-08-06T23:21:06.8697646Z /__w/pytorch/pytorch/build/../torch/csrc/jit/tensorexpr/ir.h:773:7: error: pass by value and use std::move [modernize-pass-by-value,-warnings-as-errors]
2021-08-06T23:21:06.8698782Z       const std::vector<Expr*>& params)
2021-08-06T23:21:06.8699370Z       ^~~~~~~~~~~~~~~~~~~~~~~~~
2021-08-06T23:21:06.8699659Z       std::vector<Expr*>
2021-08-06T23:21:06.8699989Z Warnings detected!
2021-08-06T23:21:06.8700348Z Summary:
2021-08-06T23:21:06.8701069Z [modernize-pass-by-value] occurred 1 times
2021-08-06T23:21:06.8701669Z     /__w/pytorch/pytorch/torch/csrc/jit/tensorexpr/ir.h:773
2021-08-06T23:21:06.8701964Z 
2021-08-06T23:21:06.8704329Z Please fix the above clang-tidy warnings.
2021-08-06T23:21:06.8764225Z ##[error]Process completed with exit code 1.

❄️ 1 failure tentatively classified as flaky

but reruns have not yet been triggered to confirm:

pytorch_linux_xenial_cuda11_1_cudnn8_py3_gcc7_test2 (1/1)

Step: "Run tests" (full log | diagnosis details | 🔁 rerun) ❄️

Aug 07 02:57:31 unknown file: Failure

Aug 07 02:57:31 frame #9: build/bin/test_api() [0xbdb3aa]
Aug 07 02:57:31 frame #10: build/bin/test_api() [0xbdbbfd]
Aug 07 02:57:31 frame #11: testing::internal::UnitTestImpl::RunAllTests() + 0xe27 (0xbe47c7 in build/bin/test_api)
Aug 07 02:57:31 frame #12: testing::UnitTest::Run() + 0x8f (0xbe4b7f in build/bin/test_api)
Aug 07 02:57:31 frame #13: main + 0xc8 (0x586cb8 in build/bin/test_api)
Aug 07 02:57:31 frame #14: __libc_start_main + 0xf0 (0x7f1f05d78840 in /lib/x86_64-linux-gnu/libc.so.6)
Aug 07 02:57:31 frame #15: _start + 0x29 (0x5c7b29 in build/bin/test_api)
Aug 07 02:57:31 " thrown in the test body.
Aug 07 02:57:31 [  FAILED  ] IMethodTest.CallMethod (4 ms)
Aug 07 02:57:31 [ RUN      ] IMethodTest.GetArgumentNames
Aug 07 02:57:31 unknown file: Failure
Aug 07 02:57:31 C++ exception with description "open file failed, file path: torch/csrc/deploy/example/generated/simple_jit
Aug 07 02:57:31 Exception raised from RAIIFile at /var/lib/jenkins/workspace/caffe2/serialize/file_adapter.cc:13 (most recent call first):
Aug 07 02:57:31 frame #0: c10::Error::Error(c10::SourceLocation, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) + 0x6b (0x7f1f0648e4ab in /var/lib/jenkins/workspace/build/lib/libc10.so)
Aug 07 02:57:31 frame #1: c10::detail::torchCheckFail(char const*, char const*, unsigned int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) + 0xce (0x7f1f06489c6e in /var/lib/jenkins/workspace/build/lib/libc10.so)
Aug 07 02:57:31 frame #2: caffe2::serialize::FileAdapter::RAIIFile::RAIIFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) + 0xdc (0x7f1f226cbdfc in /var/lib/jenkins/workspace/build/lib/libtorch_cpu.so)
Aug 07 02:57:31 frame #3: caffe2::serialize::FileAdapter::FileAdapter(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) + 0x41 (0x7f1f226cc481 in /var/lib/jenkins/workspace/build/lib/libtorch_cpu.so)
Aug 07 02:57:31 frame #4: torch::jit::load(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, c10::optional<c10::Device>, std::unordered_map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > >&) + 0x40 (0x7f1f23d27000 in /var/lib/jenkins/workspace/build/lib/libtorch_cpu.so)
Aug 07 02:57:31 frame #5: torch::jit::load(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, c10::optional<c10::Device>) + 0x6f (0x7f1f23d2718f in /var/lib/jenkins/workspace/build/lib/libtorch_cpu.so)
Aug 07 02:57:31 frame #6: IMethodTest_GetArgumentNames_Test::TestBody() + 0x7e (0xb9f5ce in build/bin/test_api)
Aug 07 02:57:31 frame #7: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) + 0x4a (0xbebb0a in build/bin/test_api)

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

navahgar · 2021-08-05T17:15:11Z

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

huiguoo · 2021-08-05T18:02:11Z

torch/csrc/jit/tensorexpr/ir_cloner.h

+class SyncThreads;
+class ExternalCall;
+
+class TORCH_API IRCloner : public IRMutator {


Should we separate IRCloner from IRMutator? They server for very different purposes and it looks weird that we do clone in the name of mutate. Since the IRCloner has its own mutations on expressions and stmts now, I guess there's not much code it can reuse from IRMutator?

Actually, I considered this and I personally didn't have a strong preference either way. And it was just simpler to keep it same as before.

If you have a strong preference to doing this, I can do it in a separate PR.

ZolotukhinM

Overall looks good, I have one question about cloning behavior with Bufs (see below).

torch/csrc/jit/tensorexpr/ir_cloner.cpp

ZolotukhinM · 2021-08-05T18:02:37Z

torch/csrc/jit/tensorexpr/ir_cloner.cpp

+    case IRNodeType::kRshift:
+      return new Rshift(lhs_new, rhs_new);
+    default:
+      throw unsupported_dtype();


Nit: we probably should throw a different error here, it's not related to dtype.

Yeah. Replaced with unimplemented_lowering.

ZolotukhinM · 2021-08-05T18:07:44Z

torch/csrc/jit/tensorexpr/ir_cloner.cpp

+  return new Load(v->dtype(), buf_new, indices_new);
+}
+
+Expr* IRCloner::mutate(Buf* v) {


That's a bit awkward that we're cloning Buf, but not cloning Vars. I understand why we might have to do this (since Buf might contain expressions in dims that need to be cloned), but it still could be a surprising behavior. Maybe for consistency we then should be cloning Vars too? Or if that's too breaking, we might consider not cloning Bufs (and its inner expressions). Either way, please document the desired behavior somewhere

I debated about this. But went ahead with cloning buffers for the exact reasons you mentioned (because the expressions in dims and initializers).

Since the difference between cloning Buf and not cloning Var could be confusing as you said, I have reverted to not cloning both of them. The only case I can think of so far which could change the expressions in dims of Buf is buffer compression. And for that case, anyways all the kernel code should be analyzed and it is better to have the clones point to the same Buf.

If something comes up later, we can revisit this assumption and add cloning for Buf and Var.

ZolotukhinM · 2021-08-05T18:09:13Z

torch/csrc/jit/tensorexpr/ir_cloner.cpp

+    indices_new.push_back(ind->accept_mutator(this));
+  }
+  auto value_new = v->value()->accept_mutator(this);
+  return new Store(v->buf(), indices_new, value_new);


We have a mutator for Buf, but we're not calling it here. Is that intentional? In what situations are we cloning the Buf?

Yeah, I missed that. Thanks for catching that. Fixed now.

…n addition to statements" Differential Revision: [D30135980](https://our.internmc.facebook.com/intern/diff/D30135980) [ghstack-poisoned]

…to statements ghstack-source-id: 624ad26 Pull Request resolved: #62833

navahgar · 2021-08-05T21:07:46Z

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

ZolotukhinM

LGTM!

…n addition to statements" Differential Revision: [D30135980](https://our.internmc.facebook.com/intern/diff/D30135980) [ghstack-poisoned]

…to statements ghstack-source-id: 3e35008 Pull Request resolved: #62833

navahgar · 2021-08-06T05:44:26Z

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

…n addition to statements" Differential Revision: [D30135980](https://our.internmc.facebook.com/intern/diff/D30135980) [ghstack-poisoned]

…to statements ghstack-source-id: b9d6884 Pull Request resolved: #62833

navahgar · 2021-08-06T16:02:46Z

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

…n addition to statements" Differential Revision: [D30135980](https://our.internmc.facebook.com/intern/diff/D30135980) [ghstack-poisoned]

…to statements ghstack-source-id: 48f9101 Pull Request resolved: #62833

navahgar · 2021-08-06T23:20:08Z

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

facebook-github-bot · 2021-08-09T16:14:36Z

@navahgar merged this pull request in e54ee9b.

[nnc] Updated IR cloning to create clones of expressions in addition …

608bea6

…to statements [ghstack-poisoned]

facebook-github-bot added the cla signed label Aug 5, 2021

navahgar requested review from ZolotukhinM and huiguoo August 5, 2021 17:16

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Aug 5, 2021

huiguoo reviewed Aug 5, 2021

View reviewed changes

ZolotukhinM reviewed Aug 5, 2021

View reviewed changes

Update on "[nnc] Updated IR cloning to create clones of expressions i…

b035a16

…n addition to statements" Differential Revision: [D30135980](https://our.internmc.facebook.com/intern/diff/D30135980) [ghstack-poisoned]

navahgar added a commit that referenced this pull request Aug 5, 2021

[nnc] Updated IR cloning to create clones of expressions in addition …

ab414a0

…to statements ghstack-source-id: 624ad26 Pull Request resolved: #62833

ZolotukhinM approved these changes Aug 5, 2021

View reviewed changes

Update on "[nnc] Updated IR cloning to create clones of expressions i…

c6a75aa

…n addition to statements" Differential Revision: [D30135980](https://our.internmc.facebook.com/intern/diff/D30135980) [ghstack-poisoned]

navahgar added a commit that referenced this pull request Aug 6, 2021

[nnc] Updated IR cloning to create clones of expressions in addition …

a6f0f72

…to statements ghstack-source-id: 3e35008 Pull Request resolved: #62833

Update on "[nnc] Updated IR cloning to create clones of expressions i…

6429dd1

…n addition to statements" Differential Revision: [D30135980](https://our.internmc.facebook.com/intern/diff/D30135980) [ghstack-poisoned]

navahgar added a commit that referenced this pull request Aug 6, 2021

[nnc] Updated IR cloning to create clones of expressions in addition …

677d79b

…to statements ghstack-source-id: b9d6884 Pull Request resolved: #62833

Update on "[nnc] Updated IR cloning to create clones of expressions i…

28dd109

…n addition to statements" Differential Revision: [D30135980](https://our.internmc.facebook.com/intern/diff/D30135980) [ghstack-poisoned]

navahgar added a commit that referenced this pull request Aug 6, 2021

[nnc] Updated IR cloning to create clones of expressions in addition …

eb1019c

…to statements ghstack-source-id: 48f9101 Pull Request resolved: #62833

facebook-github-bot closed this in e54ee9b Aug 9, 2021

facebook-github-bot added the Merged label Aug 9, 2021

facebook-github-bot deleted the gh/navahgar/45/head branch August 13, 2021 14:21

[nnc] Updated IR cloning to create clones of expressions in addition to statements #62833

[nnc] Updated IR cloning to create clones of expressions in addition to statements #62833

Uh oh!

Conversation

navahgar commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful links

💊 CI failures summary and remediations

🕵️ 1 new failure recognized by patterns

Lint / clang-tidy (1/1)

❄️ 1 failure tentatively classified as flaky

pytorch_linux_xenial_cuda11_1_cudnn8_py3_gcc7_test2 (1/1)

Uh oh!

navahgar commented Aug 5, 2021

Uh oh!

huiguoo Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

navahgar Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

ZolotukhinM left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ZolotukhinM Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

navahgar Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

ZolotukhinM Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

navahgar Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

ZolotukhinM Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

navahgar Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navahgar commented Aug 5, 2021

Uh oh!

ZolotukhinM left a comment

Choose a reason for hiding this comment

Uh oh!

navahgar commented Aug 6, 2021

Uh oh!

navahgar commented Aug 6, 2021

Uh oh!

navahgar commented Aug 6, 2021

Uh oh!

facebook-github-bot commented Aug 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

navahgar commented Aug 5, 2021 •

edited

Loading

facebook-github-bot commented Aug 5, 2021 •

edited

Loading

navahgar Aug 5, 2021 •

edited

Loading