You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/05/05 11:16:56 UTC
[GitHub] [tvm] echuraev opened a new pull request #7980: [METAL] Split kernels and compile them separately
echuraev opened a new pull request #7980:
URL: https://github.com/apache/tvm/pull/7980
Refactor Metal module to build each kernel separately. This should help
to avoid potential problem then generated blockSize is more than
maxTotalThreadsPerThreadgroup.
Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @ them in the pull request thread.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] echuraev commented on a change in pull request #7980: [METAL] Split kernels and compile them separately
Posted by GitBox <gi...@apache.org>.
echuraev commented on a change in pull request #7980:
URL: https://github.com/apache/tvm/pull/7980#discussion_r630796345
##########
File path: src/runtime/metal/metal_module.mm
##########
@@ -71,6 +73,28 @@ void SaveToBinary(dmlc::Stream* stream) final {
return "";
}
}
+
+ std::unordered_map<std::string, std::string> SplitKernels(std::string source) const {
+ std::unordered_map<std::string, std::string> split_kernels;
Review comment:
Done. Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] elvin-nnov commented on a change in pull request #7980: [METAL] Split kernels and compile them separately
Posted by GitBox <gi...@apache.org>.
elvin-nnov commented on a change in pull request #7980:
URL: https://github.com/apache/tvm/pull/7980#discussion_r629862045
##########
File path: src/runtime/metal/metal_module.mm
##########
@@ -85,44 +109,49 @@ void SaveToBinary(dmlc::Stream* stream) final {
if (it != e.smap.end()) return it->second;
// compile
NSError* err_msg = nil;
- if (e.lib == nil) {
- if (fmt_ == "metal") {
- MTLCompileOptions* opts = [MTLCompileOptions alloc];
- opts.languageVersion = MTLLanguageVersion2_3;
- opts.fastMathEnabled = YES;
- // opts = nil;
- e.lib = [w->devices[device_id]
- newLibraryWithSource:[NSString stringWithUTF8String:data_.c_str()]
- options:opts
- error:&err_msg];
- [opts dealloc];
- if (e.lib == nil) {
- LOG(FATAL) << "Fail to compile metal lib:" << [[err_msg localizedDescription] UTF8String];
- }
- if (err_msg != nil) {
- LOG(INFO) << "Warning: " << [[err_msg localizedDescription] UTF8String];
- }
- } else {
- // Build from library.
- auto q = dispatch_queue_create("q", DISPATCH_QUEUE_SERIAL);
- auto data = dispatch_data_create(data_.c_str(), data_.length(), q,
- ^{
- });
- e.lib = [w->devices[device_id] newLibraryWithData:data error:&err_msg];
- if (err_msg != nil || e.lib == nil) {
- LOG(FATAL) << "Fail to compile metal lib:" << [[err_msg localizedDescription] UTF8String];
- }
+ id<MTLLibrary> lib = nil;
+ std::string source;
+ auto kernel = parsed_kernels_.find(func_name);
+ if (kernel != parsed_kernels_.end())
Review comment:
I would add here a comment that this behaviour is done for backward compatibility when we had all kernels going together, but looks good
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] echuraev commented on a change in pull request #7980: [METAL] Split kernels and compile them separately
Posted by GitBox <gi...@apache.org>.
echuraev commented on a change in pull request #7980:
URL: https://github.com/apache/tvm/pull/7980#discussion_r631587959
##########
File path: src/runtime/source_utils.cc
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file source_utils.cc
+ */
+#include "source_utils.h"
+
+namespace tvm {
+namespace runtime {
+
+std::unordered_map<std::string, std::string> SplitKernels(std::string source,
Review comment:
Done. Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] elvin-nnov commented on a change in pull request #7980: [METAL] Split kernels and compile them separately
Posted by GitBox <gi...@apache.org>.
elvin-nnov commented on a change in pull request #7980:
URL: https://github.com/apache/tvm/pull/7980#discussion_r629862045
##########
File path: src/runtime/metal/metal_module.mm
##########
@@ -85,44 +109,49 @@ void SaveToBinary(dmlc::Stream* stream) final {
if (it != e.smap.end()) return it->second;
// compile
NSError* err_msg = nil;
- if (e.lib == nil) {
- if (fmt_ == "metal") {
- MTLCompileOptions* opts = [MTLCompileOptions alloc];
- opts.languageVersion = MTLLanguageVersion2_3;
- opts.fastMathEnabled = YES;
- // opts = nil;
- e.lib = [w->devices[device_id]
- newLibraryWithSource:[NSString stringWithUTF8String:data_.c_str()]
- options:opts
- error:&err_msg];
- [opts dealloc];
- if (e.lib == nil) {
- LOG(FATAL) << "Fail to compile metal lib:" << [[err_msg localizedDescription] UTF8String];
- }
- if (err_msg != nil) {
- LOG(INFO) << "Warning: " << [[err_msg localizedDescription] UTF8String];
- }
- } else {
- // Build from library.
- auto q = dispatch_queue_create("q", DISPATCH_QUEUE_SERIAL);
- auto data = dispatch_data_create(data_.c_str(), data_.length(), q,
- ^{
- });
- e.lib = [w->devices[device_id] newLibraryWithData:data error:&err_msg];
- if (err_msg != nil || e.lib == nil) {
- LOG(FATAL) << "Fail to compile metal lib:" << [[err_msg localizedDescription] UTF8String];
- }
+ id<MTLLibrary> lib = nil;
+ std::string source;
+ auto kernel = parsed_kernels_.find(func_name);
+ if (kernel != parsed_kernels_.end())
Review comment:
I would add here a comment that this behaviour is done for backward compatibility when we had all kernels going together without explicit separators, but looks good
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] csullivan commented on a change in pull request #7980: [METAL] Split kernels and compile them separately
Posted by GitBox <gi...@apache.org>.
csullivan commented on a change in pull request #7980:
URL: https://github.com/apache/tvm/pull/7980#discussion_r631926225
##########
File path: src/runtime/metal/metal_module.mm
##########
@@ -85,44 +109,49 @@ void SaveToBinary(dmlc::Stream* stream) final {
if (it != e.smap.end()) return it->second;
// compile
NSError* err_msg = nil;
- if (e.lib == nil) {
- if (fmt_ == "metal") {
- MTLCompileOptions* opts = [MTLCompileOptions alloc];
- opts.languageVersion = MTLLanguageVersion2_3;
- opts.fastMathEnabled = YES;
- // opts = nil;
- e.lib = [w->devices[device_id]
- newLibraryWithSource:[NSString stringWithUTF8String:data_.c_str()]
- options:opts
- error:&err_msg];
- [opts dealloc];
- if (e.lib == nil) {
- LOG(FATAL) << "Fail to compile metal lib:" << [[err_msg localizedDescription] UTF8String];
- }
- if (err_msg != nil) {
- LOG(INFO) << "Warning: " << [[err_msg localizedDescription] UTF8String];
- }
- } else {
- // Build from library.
- auto q = dispatch_queue_create("q", DISPATCH_QUEUE_SERIAL);
- auto data = dispatch_data_create(data_.c_str(), data_.length(), q,
- ^{
- });
- e.lib = [w->devices[device_id] newLibraryWithData:data error:&err_msg];
- if (err_msg != nil || e.lib == nil) {
- LOG(FATAL) << "Fail to compile metal lib:" << [[err_msg localizedDescription] UTF8String];
- }
+ id<MTLLibrary> lib = nil;
+ std::string source;
+ auto kernel = parsed_kernels_.find(func_name);
+ if (kernel != parsed_kernels_.end())
Review comment:
Do we have metal deployments that require the old behavior that can't be rebuilt easily? Otherwise we don't need to preserve backward compatibility.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] echuraev commented on a change in pull request #7980: [METAL] Split kernels and compile them separately
Posted by GitBox <gi...@apache.org>.
echuraev commented on a change in pull request #7980:
URL: https://github.com/apache/tvm/pull/7980#discussion_r631585533
##########
File path: src/runtime/source_utils.cc
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file source_utils.cc
+ */
+#include "source_utils.h"
+
+namespace tvm {
+namespace runtime {
+
+std::unordered_map<std::string, std::string> SplitKernels(std::string source,
+ std::string delimiter) {
+ std::unordered_map<std::string, std::string> split_kernels;
+ if (source.size()) {
+ size_t begin = source.find(delimiter);
Review comment:
I did it to keep backward compatibility as @elvin-nnov requested. In case when the Metal library was generated before this change, we won't find any delimiters in the source code.
But I added some checks to opencl module:
https://github.com/apache/tvm/pull/7980/files#diff-9d619c5f05402f93afa552f93675dea84d789f8f6b0c553845aa183128f38494R192-R196
Also, I added check in metal module were kernels found or not: https://github.com/apache/tvm/pull/7980/files#diff-dc30040e2ec938d8dd0e6e87d6517870f037093775d4b52e0f249fdbebfdb9a3R95-R101
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #7980: [METAL] Split kernels and compile them separately
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7980:
URL: https://github.com/apache/tvm/pull/7980#discussion_r631007548
##########
File path: src/runtime/source_utils.cc
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file source_utils.cc
+ */
+#include "source_utils.h"
+
+namespace tvm {
+namespace runtime {
+
+std::unordered_map<std::string, std::string> SplitKernels(std::string source,
Review comment:
add include of source_utils.cc to
- https://github.com/apache/tvm/blob/main/apps/android_rpc/app/src/main/jni/tvm_runtime.h#L64
- https://github.com/apache/tvm/blob/6eedad1e386fe4e8c8930324a5a5e7102bb28d25/golang/src/tvm_runtime_pack.cc#L70
- same file on apps/android_camera
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #7980: [METAL] Split kernels and compile them separately
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7980:
URL: https://github.com/apache/tvm/pull/7980#discussion_r631007548
##########
File path: src/runtime/source_utils.cc
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file source_utils.cc
+ */
+#include "source_utils.h"
+
+namespace tvm {
+namespace runtime {
+
+std::unordered_map<std::string, std::string> SplitKernels(std::string source,
Review comment:
Please add include of source_utils.cc to
- https://github.com/apache/tvm/blob/main/apps/android_rpc/app/src/main/jni/tvm_runtime.h#L64
- https://github.com/apache/tvm/blob/6eedad1e386fe4e8c8930324a5a5e7102bb28d25/golang/src/tvm_runtime_pack.cc#L70
- same file on apps/android_camera
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #7980: [METAL] Split kernels and compile them separately
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7980:
URL: https://github.com/apache/tvm/pull/7980#discussion_r631007548
##########
File path: src/runtime/source_utils.cc
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file source_utils.cc
+ */
+#include "source_utils.h"
+
+namespace tvm {
+namespace runtime {
+
+std::unordered_map<std::string, std::string> SplitKernels(std::string source,
Review comment:
add include of source_utils.cc to
https://github.com/apache/tvm/blob/main/apps/android_rpc/app/src/main/jni/tvm_runtime.h#L64
https://github.com/apache/tvm/blob/6eedad1e386fe4e8c8930324a5a5e7102bb28d25/golang/src/tvm_runtime_pack.cc#L70
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #7980: [METAL] Split kernels and compile them separately
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7980:
URL: https://github.com/apache/tvm/pull/7980#discussion_r630173046
##########
File path: src/runtime/metal/metal_module.mm
##########
@@ -71,6 +73,28 @@ void SaveToBinary(dmlc::Stream* stream) final {
return "";
}
}
+
+ std::unordered_map<std::string, std::string> SplitKernels(std::string source) const {
+ std::unordered_map<std::string, std::string> split_kernels;
Review comment:
cc @csullivan
##########
File path: src/runtime/metal/metal_module.mm
##########
@@ -71,6 +73,28 @@ void SaveToBinary(dmlc::Stream* stream) final {
return "";
}
}
+
+ std::unordered_map<std::string, std::string> SplitKernels(std::string source) const {
+ std::unordered_map<std::string, std::string> split_kernels;
Review comment:
This is a duplicated util from https://github.com/apache/tvm/blob/main/src/runtime/opencl/opencl_module.cc#L245. consider move to a common one. e.g. src/runtime/source_utils.h
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] csullivan commented on a change in pull request #7980: [METAL] Split kernels and compile them separately
Posted by GitBox <gi...@apache.org>.
csullivan commented on a change in pull request #7980:
URL: https://github.com/apache/tvm/pull/7980#discussion_r632152025
##########
File path: src/runtime/metal/metal_module.mm
##########
@@ -85,44 +109,49 @@ void SaveToBinary(dmlc::Stream* stream) final {
if (it != e.smap.end()) return it->second;
// compile
NSError* err_msg = nil;
- if (e.lib == nil) {
- if (fmt_ == "metal") {
- MTLCompileOptions* opts = [MTLCompileOptions alloc];
- opts.languageVersion = MTLLanguageVersion2_3;
- opts.fastMathEnabled = YES;
- // opts = nil;
- e.lib = [w->devices[device_id]
- newLibraryWithSource:[NSString stringWithUTF8String:data_.c_str()]
- options:opts
- error:&err_msg];
- [opts dealloc];
- if (e.lib == nil) {
- LOG(FATAL) << "Fail to compile metal lib:" << [[err_msg localizedDescription] UTF8String];
- }
- if (err_msg != nil) {
- LOG(INFO) << "Warning: " << [[err_msg localizedDescription] UTF8String];
- }
- } else {
- // Build from library.
- auto q = dispatch_queue_create("q", DISPATCH_QUEUE_SERIAL);
- auto data = dispatch_data_create(data_.c_str(), data_.length(), q,
- ^{
- });
- e.lib = [w->devices[device_id] newLibraryWithData:data error:&err_msg];
- if (err_msg != nil || e.lib == nil) {
- LOG(FATAL) << "Fail to compile metal lib:" << [[err_msg localizedDescription] UTF8String];
- }
+ id<MTLLibrary> lib = nil;
+ std::string source;
+ auto kernel = parsed_kernels_.find(func_name);
+ if (kernel != parsed_kernels_.end())
Review comment:
In that case I personally would prefer the metal backward compat be specific to the metal backend and the general SplitKernel util to contain the existence ICHECK on the function delimiter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a change in pull request #7980: [METAL] Split kernels and compile them separately
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7980:
URL: https://github.com/apache/tvm/pull/7980#discussion_r630840348
##########
File path: src/runtime/metal/metal_module.mm
##########
@@ -43,7 +44,9 @@
public:
explicit MetalModuleNode(std::string data, std::string fmt,
std::unordered_map<std::string, FunctionInfo> fmap, std::string source)
- : data_(data), fmt_(fmt), fmap_(fmap), source_(source) {}
+ : data_(data), fmt_(fmt), fmap_(fmap), source_(source) {
+ parsed_kernels_ = SplitKernels(GetSource(fmt_));
Review comment:
How about passing `data` directly here to `SplitKernels`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] echuraev commented on a change in pull request #7980: [METAL] Split kernels and compile them separately
Posted by GitBox <gi...@apache.org>.
echuraev commented on a change in pull request #7980:
URL: https://github.com/apache/tvm/pull/7980#discussion_r627087841
##########
File path: src/runtime/metal/metal_module.mm
##########
@@ -85,44 +112,45 @@ void SaveToBinary(dmlc::Stream* stream) final {
if (it != e.smap.end()) return it->second;
// compile
NSError* err_msg = nil;
- if (e.lib == nil) {
- if (fmt_ == "metal") {
- MTLCompileOptions* opts = [MTLCompileOptions alloc];
- opts.languageVersion = MTLLanguageVersion2_3;
- opts.fastMathEnabled = YES;
- // opts = nil;
- e.lib = [w->devices[device_id]
- newLibraryWithSource:[NSString stringWithUTF8String:data_.c_str()]
- options:opts
- error:&err_msg];
- [opts dealloc];
- if (e.lib == nil) {
- LOG(FATAL) << "Fail to compile metal lib:" << [[err_msg localizedDescription] UTF8String];
- }
- if (err_msg != nil) {
- LOG(INFO) << "Warning: " << [[err_msg localizedDescription] UTF8String];
- }
- } else {
- // Build from library.
- auto q = dispatch_queue_create("q", DISPATCH_QUEUE_SERIAL);
- auto data = dispatch_data_create(data_.c_str(), data_.length(), q,
- ^{
- });
- e.lib = [w->devices[device_id] newLibraryWithData:data error:&err_msg];
- if (err_msg != nil || e.lib == nil) {
- LOG(FATAL) << "Fail to compile metal lib:" << [[err_msg localizedDescription] UTF8String];
- }
+ id<MTLLibrary> lib = nil;
+ if (fmt_ == "metal") {
+ MTLCompileOptions* opts = [MTLCompileOptions alloc];
+ opts.languageVersion = MTLLanguageVersion2_3;
+ opts.fastMathEnabled = YES;
+ // opts = nil;
+ std::string source = parsed_kernels_[func_name];
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] elvin-nnov commented on a change in pull request #7980: [METAL] Split kernels and compile them separately
Posted by GitBox <gi...@apache.org>.
elvin-nnov commented on a change in pull request #7980:
URL: https://github.com/apache/tvm/pull/7980#discussion_r632087396
##########
File path: src/runtime/metal/metal_module.mm
##########
@@ -85,44 +109,49 @@ void SaveToBinary(dmlc::Stream* stream) final {
if (it != e.smap.end()) return it->second;
// compile
NSError* err_msg = nil;
- if (e.lib == nil) {
- if (fmt_ == "metal") {
- MTLCompileOptions* opts = [MTLCompileOptions alloc];
- opts.languageVersion = MTLLanguageVersion2_3;
- opts.fastMathEnabled = YES;
- // opts = nil;
- e.lib = [w->devices[device_id]
- newLibraryWithSource:[NSString stringWithUTF8String:data_.c_str()]
- options:opts
- error:&err_msg];
- [opts dealloc];
- if (e.lib == nil) {
- LOG(FATAL) << "Fail to compile metal lib:" << [[err_msg localizedDescription] UTF8String];
- }
- if (err_msg != nil) {
- LOG(INFO) << "Warning: " << [[err_msg localizedDescription] UTF8String];
- }
- } else {
- // Build from library.
- auto q = dispatch_queue_create("q", DISPATCH_QUEUE_SERIAL);
- auto data = dispatch_data_create(data_.c_str(), data_.length(), q,
- ^{
- });
- e.lib = [w->devices[device_id] newLibraryWithData:data error:&err_msg];
- if (err_msg != nil || e.lib == nil) {
- LOG(FATAL) << "Fail to compile metal lib:" << [[err_msg localizedDescription] UTF8String];
- }
+ id<MTLLibrary> lib = nil;
+ std::string source;
+ auto kernel = parsed_kernels_.find(func_name);
+ if (kernel != parsed_kernels_.end())
Review comment:
I cannot say for sure. I can imagine some use cases which might have issues with recompilation, but this is just assumptions.
If we do not declare backward compatibility for now, we can skip my comment and return original behaviour and rise an exception if we do not find kernel in `parsed_kernels_` container.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen merged pull request #7980: [METAL] Split kernels and compile them separately
Posted by GitBox <gi...@apache.org>.
tqchen merged pull request #7980:
URL: https://github.com/apache/tvm/pull/7980
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on pull request #7980: [METAL] Split kernels and compile them separately
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #7980:
URL: https://github.com/apache/tvm/pull/7980#issuecomment-838487632
cc @masahi @csullivan please help to review the PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] elvin-nnov commented on a change in pull request #7980: [METAL] Split kernels and compile them separately
Posted by GitBox <gi...@apache.org>.
elvin-nnov commented on a change in pull request #7980:
URL: https://github.com/apache/tvm/pull/7980#discussion_r626489762
##########
File path: src/runtime/metal/metal_module.mm
##########
@@ -85,44 +112,45 @@ void SaveToBinary(dmlc::Stream* stream) final {
if (it != e.smap.end()) return it->second;
// compile
NSError* err_msg = nil;
- if (e.lib == nil) {
- if (fmt_ == "metal") {
- MTLCompileOptions* opts = [MTLCompileOptions alloc];
- opts.languageVersion = MTLLanguageVersion2_3;
- opts.fastMathEnabled = YES;
- // opts = nil;
- e.lib = [w->devices[device_id]
- newLibraryWithSource:[NSString stringWithUTF8String:data_.c_str()]
- options:opts
- error:&err_msg];
- [opts dealloc];
- if (e.lib == nil) {
- LOG(FATAL) << "Fail to compile metal lib:" << [[err_msg localizedDescription] UTF8String];
- }
- if (err_msg != nil) {
- LOG(INFO) << "Warning: " << [[err_msg localizedDescription] UTF8String];
- }
- } else {
- // Build from library.
- auto q = dispatch_queue_create("q", DISPATCH_QUEUE_SERIAL);
- auto data = dispatch_data_create(data_.c_str(), data_.length(), q,
- ^{
- });
- e.lib = [w->devices[device_id] newLibraryWithData:data error:&err_msg];
- if (err_msg != nil || e.lib == nil) {
- LOG(FATAL) << "Fail to compile metal lib:" << [[err_msg localizedDescription] UTF8String];
- }
+ id<MTLLibrary> lib = nil;
+ if (fmt_ == "metal") {
+ MTLCompileOptions* opts = [MTLCompileOptions alloc];
+ opts.languageVersion = MTLLanguageVersion2_3;
+ opts.fastMathEnabled = YES;
+ // opts = nil;
+ std::string source = parsed_kernels_[func_name];
Review comment:
let's add here backward compatible behaviour - if we do not find kernel name in parsed_kernels_, it will be useful to feed the whole `data_` as it was before
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] echuraev commented on a change in pull request #7980: [METAL] Split kernels and compile them separately
Posted by GitBox <gi...@apache.org>.
echuraev commented on a change in pull request #7980:
URL: https://github.com/apache/tvm/pull/7980#discussion_r630788030
##########
File path: src/runtime/metal/metal_module.mm
##########
@@ -85,44 +109,49 @@ void SaveToBinary(dmlc::Stream* stream) final {
if (it != e.smap.end()) return it->second;
// compile
NSError* err_msg = nil;
- if (e.lib == nil) {
- if (fmt_ == "metal") {
- MTLCompileOptions* opts = [MTLCompileOptions alloc];
- opts.languageVersion = MTLLanguageVersion2_3;
- opts.fastMathEnabled = YES;
- // opts = nil;
- e.lib = [w->devices[device_id]
- newLibraryWithSource:[NSString stringWithUTF8String:data_.c_str()]
- options:opts
- error:&err_msg];
- [opts dealloc];
- if (e.lib == nil) {
- LOG(FATAL) << "Fail to compile metal lib:" << [[err_msg localizedDescription] UTF8String];
- }
- if (err_msg != nil) {
- LOG(INFO) << "Warning: " << [[err_msg localizedDescription] UTF8String];
- }
- } else {
- // Build from library.
- auto q = dispatch_queue_create("q", DISPATCH_QUEUE_SERIAL);
- auto data = dispatch_data_create(data_.c_str(), data_.length(), q,
- ^{
- });
- e.lib = [w->devices[device_id] newLibraryWithData:data error:&err_msg];
- if (err_msg != nil || e.lib == nil) {
- LOG(FATAL) << "Fail to compile metal lib:" << [[err_msg localizedDescription] UTF8String];
- }
+ id<MTLLibrary> lib = nil;
+ std::string source;
+ auto kernel = parsed_kernels_.find(func_name);
+ if (kernel != parsed_kernels_.end())
Review comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] csullivan commented on a change in pull request #7980: [METAL] Split kernels and compile them separately
Posted by GitBox <gi...@apache.org>.
csullivan commented on a change in pull request #7980:
URL: https://github.com/apache/tvm/pull/7980#discussion_r631280030
##########
File path: src/runtime/source_utils.cc
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file source_utils.cc
+ */
+#include "source_utils.h"
+
+namespace tvm {
+namespace runtime {
+
+std::unordered_map<std::string, std::string> SplitKernels(std::string source,
+ std::string delimiter) {
+ std::unordered_map<std::string, std::string> split_kernels;
+ if (source.size()) {
+ size_t begin = source.find(delimiter);
Review comment:
I noticed you deleted ICHECK here, probably because the error message referred to OpenCL specifically, but there still should be an
```
ICHECK(begin != std::string::npos)
```
on the first search for a kernel delimiter. If none are found, then it is better to catch an error in the code generation early.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] elvin-nnov commented on a change in pull request #7980: [METAL] Split kernels and compile them separately
Posted by GitBox <gi...@apache.org>.
elvin-nnov commented on a change in pull request #7980:
URL: https://github.com/apache/tvm/pull/7980#discussion_r632299781
##########
File path: src/runtime/metal/metal_module.mm
##########
@@ -85,44 +109,49 @@ void SaveToBinary(dmlc::Stream* stream) final {
if (it != e.smap.end()) return it->second;
// compile
NSError* err_msg = nil;
- if (e.lib == nil) {
- if (fmt_ == "metal") {
- MTLCompileOptions* opts = [MTLCompileOptions alloc];
- opts.languageVersion = MTLLanguageVersion2_3;
- opts.fastMathEnabled = YES;
- // opts = nil;
- e.lib = [w->devices[device_id]
- newLibraryWithSource:[NSString stringWithUTF8String:data_.c_str()]
- options:opts
- error:&err_msg];
- [opts dealloc];
- if (e.lib == nil) {
- LOG(FATAL) << "Fail to compile metal lib:" << [[err_msg localizedDescription] UTF8String];
- }
- if (err_msg != nil) {
- LOG(INFO) << "Warning: " << [[err_msg localizedDescription] UTF8String];
- }
- } else {
- // Build from library.
- auto q = dispatch_queue_create("q", DISPATCH_QUEUE_SERIAL);
- auto data = dispatch_data_create(data_.c_str(), data_.length(), q,
- ^{
- });
- e.lib = [w->devices[device_id] newLibraryWithData:data error:&err_msg];
- if (err_msg != nil || e.lib == nil) {
- LOG(FATAL) << "Fail to compile metal lib:" << [[err_msg localizedDescription] UTF8String];
- }
+ id<MTLLibrary> lib = nil;
+ std::string source;
+ auto kernel = parsed_kernels_.find(func_name);
+ if (kernel != parsed_kernels_.end())
Review comment:
@tqchen do we care about backward compatibility or not? If not, it might be ok so far, but we need to decide when we will start to care and what use cases should work and be regularly verified in the future. It must not be a deal only metal backend
@csullivan what bahavior do you propose in common `SplitKernel`? Metal backend uses it, it cannot just abort execution. Warnings in the runtime are also useles. It might be an error code, but empty returned map detects the absent of Function delimiter with the same efficiency as return code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] csullivan commented on a change in pull request #7980: [METAL] Split kernels and compile them separately
Posted by GitBox <gi...@apache.org>.
csullivan commented on a change in pull request #7980:
URL: https://github.com/apache/tvm/pull/7980#discussion_r632152200
##########
File path: src/runtime/source_utils.cc
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file source_utils.cc
+ */
+#include "source_utils.h"
+
+namespace tvm {
+namespace runtime {
+
+std::unordered_map<std::string, std::string> SplitKernels(std::string source,
+ std::string delimiter) {
+ std::unordered_map<std::string, std::string> split_kernels;
+ if (source.size()) {
+ size_t begin = source.find(delimiter);
Review comment:
I would prefer the metal backward compat be specific to the metal backend and the general SplitKernel util to contain the existence ICHECK on the function delimiter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] echuraev commented on a change in pull request #7980: [METAL] Split kernels and compile them separately
Posted by GitBox <gi...@apache.org>.
echuraev commented on a change in pull request #7980:
URL: https://github.com/apache/tvm/pull/7980#discussion_r630889037
##########
File path: src/runtime/metal/metal_module.mm
##########
@@ -43,7 +44,9 @@
public:
explicit MetalModuleNode(std::string data, std::string fmt,
std::unordered_map<std::string, FunctionInfo> fmap, std::string source)
- : data_(data), fmt_(fmt), fmap_(fmap), source_(source) {}
+ : data_(data), fmt_(fmt), fmap_(fmap), source_(source) {
+ parsed_kernels_ = SplitKernels(GetSource(fmt_));
Review comment:
You are right. Thank you. We can use `data_` here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #7980: [METAL] Split kernels and compile them separately
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7980:
URL: https://github.com/apache/tvm/pull/7980#issuecomment-839635870
@tqchen please take a look again
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org