-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Relay] Prepare for new plan_devices.cc (part II) #9130
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
162c422
Prepare for new plan_devices.cc (part II)
mbs-octoml 3cbeb81
[checkpoint] Keep existing ParseModule ffi to simplify rust bindings
mbs-octoml 4d7c798
[checkpoint] Address Christopher's comments.
mbs-octoml 5553be2
[checkpoint] Andrew's comments from #9038
mbs-octoml 738bb29
[checkpoint] Jared's comments from #9038
mbs-octoml 72b0784
[checkpoint] Woops, forgot rename.
mbs-octoml File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1092,8 +1092,6 @@ class Parser { | |
|
||
Array<TypeVar> generics; | ||
if (Peek()->token_type == TokenType::kLSquare) { | ||
// If we have generics we need to add a type scope. | ||
PushTypeScope(); | ||
generics = ParseSequence<TypeVar>( | ||
TokenType::kLSquare, TokenType::kComma, TokenType::kRSquare, [&]() { | ||
auto type_var_name = Match(TokenType::kIdentifier).ToString(); | ||
|
@@ -1444,6 +1442,10 @@ class Parser { | |
ICHECK(attr_obj.defined()); | ||
attrs = Downcast<Attrs>(attr_obj); | ||
} | ||
} else { | ||
this->diag_ctx.EmitFatal(Diagnostic::Error(op->span) | ||
<< "unable to determine the 'attrs_type_key' with which " | ||
"to represent the call attributes for this operator"); | ||
} | ||
} | ||
return true; | ||
|
@@ -1867,7 +1869,7 @@ class Parser { | |
}; | ||
|
||
Parser InitParser(const std::string& file_name, const std::string& file_content, | ||
Optional<IRModule> init_module) { | ||
const Optional<IRModule>& init_module, const MetaTable& init_meta_table) { | ||
VLOG(0) << "InitParser: file_name: " << file_name << "file_content_size: " << file_content.size(); | ||
SourceName src_name = SourceName::Get(file_name); | ||
Source source(src_name, file_content); | ||
|
@@ -1886,42 +1888,62 @@ Parser InitParser(const std::string& file_name, const std::string& file_content, | |
auto tokens_and_table = Tokenize(diag_ctx, source); | ||
|
||
auto tokens = tokens_and_table.first; | ||
auto meta_data_table = tokens_and_table.second; | ||
MetaTable meta_data_table = tokens_and_table.second.ToMetadata(); | ||
|
||
// Merge any entries in init_meta_table into anything captured in the #[metadata] section | ||
// of the file_content. Metadata references within file_content must use indexes which account | ||
// for this ordering. | ||
for (const auto& pair : init_meta_table) { | ||
Array<ObjectRef> items; | ||
if (meta_data_table.count(pair.first)) { | ||
items = meta_data_table[pair.first]; | ||
} | ||
for (const auto& obj : pair.second) { | ||
items.push_back(obj); | ||
} | ||
meta_data_table.Set(pair.first, items); | ||
} | ||
Comment on lines
+1896
to
+1905
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 can't see any test cases for this logic, is it possible for you to add one so I can see it in action? 😸 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. Good call - done. |
||
|
||
return Parser(module, diag_ctx, source, tokens, DefaultOpTable(), meta_data_table.ToMetadata()); | ||
return Parser(module, diag_ctx, source, tokens, DefaultOpTable(), std::move(meta_data_table)); | ||
} | ||
|
||
IRModule ParseModule(std::string file_name, std::string file_content, | ||
Optional<IRModule> init_module) { | ||
IRModule ParseModule(const std::string& file_name, const std::string& file_content, | ||
const Optional<IRModule>& init_module, const MetaTable& init_meta_table) { | ||
VLOG(0) << "ParseModule"; | ||
auto parser = InitParser(file_name, file_content, init_module); | ||
auto parser = InitParser(file_name, file_content, init_module, init_meta_table); | ||
auto mod = parser.ParseModule(); | ||
ICHECK(mod.defined()) << "The parser must return a non-null module."; | ||
// NB(@jroesch): it is very important that we render any errors before we procede | ||
// if there were any errors which allow the parser to procede we must render them | ||
// NB(@jroesch): it is very important that we render any errors before we proceed | ||
// if there were any errors which allow the parser to proceed we must render them | ||
// here. | ||
parser.diag_ctx.Render(); | ||
auto infer_type = tvm::relay::transform::InferType(); | ||
ICHECK(infer_type.defined()) << "The type inferencer must be non-null."; | ||
return infer_type(mod); | ||
} | ||
|
||
Expr ParseExpr(std::string file_name, std::string file_content) { | ||
Expr ParseExpr(const std::string& file_name, const std::string& file_content) { | ||
VLOG(0) << "ParseExpr"; | ||
auto parser = InitParser(file_name, file_content, Optional<IRModule>()); | ||
auto parser = InitParser(file_name, file_content, Optional<IRModule>(), MetaTable()); | ||
parser.ParseSemVer(false); | ||
parser.PushScope(); | ||
auto expr = parser.ParseExpr(); | ||
parser.Match(TokenType::kEndOfFile); | ||
// NB(@jroesch): it is very important that we render any errors before we procede | ||
// if there were any errors which allow the parser to procede we must render them | ||
// NB(@jroesch): it is very important that we render any errors before we proceed | ||
// if there were any errors which allow the parser to proceed we must render them | ||
// here. | ||
parser.diag_ctx.Render(); | ||
return expr; | ||
} | ||
|
||
TVM_REGISTER_GLOBAL("parser.ParseModuleInContext") | ||
.set_body_typed([](const std::string& file_name, const std::string& file_content, | ||
const Optional<IRModule>& init_module, const MetaTable& init_meta_table) { | ||
return ParseModule(file_name, file_content, init_module, init_meta_table); | ||
}); | ||
|
||
TVM_REGISTER_GLOBAL("parser.ParseModule") | ||
.set_body_typed([](tvm::String file_name, tvm::String file_content) { | ||
.set_body_typed([](const std::string& file_name, const std::string& file_content) { | ||
return ParseModule(file_name, file_content); | ||
}); | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This can be defaulted in the arguments.
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. (I had the obvious default but clion was warning about mutable defaults and suggest this instead.)
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.
Oh, the linter complains about it too "Dangerous default value {} as argument" - I'll go back to the silly form.
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.
You can't default Python arguments to anything but atomic values. If you use an object or other aggregate data structure the default will be allocated a single time, and only a single time. If you happen to mutate it you will observe the entire history of mutations across all invocations of the function inside the process.
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.
Ah, thanks! Not so silly a rule after all.
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'd suggest the language behaviour is the silly part? 👍
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.
Yeah its not a good design, but alas you gotta love the Python you are with 😆 it has burned many people many times, super sharp edge.