sunggg commented on code in PR #12626:
URL: https://github.com/apache/tvm/pull/12626#discussion_r957428726
##########
tests/python/unittest/test_link_params.py:
##########
@@ -407,21 +406,21 @@ def schedule_dense(sch):
target = "llvm"
params = {"weight": weight_np}
- def schedule_fn(task, sch):
- if "nn_dense" in task.task_name:
+ def schedule_fn(sch):
Review Comment:
I'm okay with this for now and agree that this delivers the functionality we
need. But like we discussed offline, I have some concerns about this approach
and hope to have some discussion in the future PRs.
- If developers start writing more target-specific, layout-specific manual
schedules, we may end up facing complicated if-statement logics that might be
hard to maintain.
- Conditional statement are based on the task name, so when task name is
different from what we expect, it wouldn't work. When task naming rule changes,
developers might need to update this accordingly, which is not intuitive imo.
- I agree with @MasterJH5574, I'm not sure if this is intuitive interface
for developers. You would need to modify the schedule and return `True`. Wonder
if we can skip manually returning true by automatically checking if the
schedule has changed.
##########
src/meta_schedule/database/database.cc:
##########
@@ -156,7 +156,8 @@ TuningRecord TuningRecord::FromJSON(const ObjectRef&
json_obj, const Workload& w
/******** Database ********/
-Optional<TuningRecord> DatabaseNode::QueryTuningRecord(IRModule mod, Target
target) {
+Optional<TuningRecord> DatabaseNode::QueryTuningRecord(const IRModule& mod,
const Target& target,
+ const String&
workload_name) {
Review Comment:
Why do we pass `workload_name`? The these three functions do not seem to use
this: `QueryTuningRecord`, `QuerySchedule`, `QueryIRModule`.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
