-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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.
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
[METAL] Split kernels and compile them separately #7980
Changes from all commits
b0b51fd
4938d70
070bba4
8041a29
df55c33
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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, | ||
echuraev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
std::string delimiter) { | ||
std::unordered_map<std::string, std::string> split_kernels; | ||
if (source.size()) { | ||
size_t begin = source.find(delimiter); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed you deleted ICHECK here, probably because the error message referred to OpenCL specifically, but there still should be an
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Also, I added check in metal module were kernels found or not: https://github.com/apache/tvm/pull/7980/files#diff-dc30040e2ec938d8dd0e6e87d6517870f037093775d4b52e0f249fdbebfdb9a3R95-R101 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
size_t end = begin; | ||
while (end != std::string::npos) { | ||
begin += delimiter.size(); | ||
end = source.find('\n', begin); | ||
std::string func_name = source.substr(begin, end - begin); | ||
begin = ++end; | ||
end = source.find(delimiter, begin); | ||
std::string func_source = | ||
source.substr(begin, (end == std::string::npos) ? end : end - begin); | ||
split_kernels.insert({func_name, func_source}); | ||
begin = end; | ||
} | ||
} | ||
return split_kernels; | ||
} | ||
} // namespace runtime | ||
} // namespace tvm |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
/* | ||
* 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.h | ||
* \brief Minimum source manipulation utils for runtime. | ||
*/ | ||
|
||
#ifndef TVM_RUNTIME_SOURCE_UTILS_H_ | ||
#define TVM_RUNTIME_SOURCE_UTILS_H_ | ||
|
||
#include <string> | ||
#include <unordered_map> | ||
|
||
namespace tvm { | ||
namespace runtime { | ||
/*! | ||
* \brief Split the source file on separate kernels by specified delimiter. | ||
* \param source The source code of the kernels. | ||
* \param delimiter The delimiter which is using for splitting kernels. | ||
* \return Mapping from primitive name to kernel source | ||
*/ | ||
std::unordered_map<std::string, std::string> SplitKernels(std::string source, | ||
std::string delimiter = "// Function: "); | ||
} // namespace runtime | ||
} // namespace tvm | ||
|
||
#endif // TVM_RUNTIME_SOURCE_UTILS_H_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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