Add order to functions in C Codegen #10590
Conversation
| @@ -132,6 +132,7 @@ PackedFunc AotExecutor::GetFunction(const std::string& name, | |||
| *rv = this->GetInputIndex(args[0].operator String()); | |||
| }); | |||
| } else { | |||
| LOG(INFO) << "Unknown packed function: " << name; | |||
There was a problem hiding this comment.
for this one, i think return PackedFunc() is still abiding by the GetFunction convention, so it might be a bit superfluous to emit a log line here.
There was a problem hiding this comment.
I'm looking for a way to show some output message to developers that they are using the wrong function name for this module. Specially since we have multiple version of this runtime.Module it would be very useful to have some messaging. What is your suggestion?
Also is LOG(DEBUG) better in this case?
There was a problem hiding this comment.
does this work? or are you saying from c++?
There was a problem hiding this comment.
yeah from C++. I would overwrite << operator in runtime::Module to provide more info on this. Cause that way we can get module type in C++.
| @@ -105,7 +105,7 @@ PackedFunc Executable::GetFunction(const std::string& name, const ObjectPtr<Obje | |||
| }); | |||
| } else { | |||
| LOG(FATAL) << "Unknown packed function: " << name; | |||
| return PackedFunc(nullptr); | |||
| return PackedFunc(); | |||
There was a problem hiding this comment.
i think this change is equivalent
There was a problem hiding this comment.
yeah, PackedFunc() just matches with the rest of the codebase.
src/target/source/codegen_c_host.h
Outdated
| @@ -42,7 +43,12 @@ class CodeGenCHost : public CodeGenC { | |||
|
|
|||
| void InitGlobalContext(); | |||
| void AddFunction(const PrimFunc& f); | |||
|
|
|||
| /*! | |||
| * \brief Add functions from the (unordered) range to the current module in a deterministic order. | |||
There was a problem hiding this comment.
a vector is ordered, so what do you mean by unordered?
There was a problem hiding this comment.
name of the functions in the vector are not ordered.
|
@areusch CI is green, PTAL! |
|
@areusch Addressed comments and added another test for the function order. PTAL! |
tests/cpp/c_codegen_test.cc
Outdated
| schedule_names.push_back(func.first->name_hint); | ||
| } | ||
| } | ||
| EXPECT_THAT(schedule_names, ElementsAre(StrEq("op_2"), StrEq("op_1"))); |
There was a problem hiding this comment.
sorry to ask for a second change--this is a little bit risky as the insertion order of unordered_map isn't guaranteed. this test could start randomly failing and be hard to reproduce. i think a more stable approach would be to wrap the logic that creates s1/s2 into a functor and call it at least 2 times (e.g. creating "op_9", "op_8", ...) and then continue calling it until the map is not sorted. then do the remainder. would you mind making that change? sorry about this.
|
@areusch addressed comments, PTAL. thanks! |
|
thanks @mehrdadh ! |
* Add function ordering to C Codegen * trigger * fix comment * address comments * add test * add unorder check * fix test * address comments
This PR add ordering to functions in C codegen. Also, it add few logging messages.
cc @areusch