tqchen commented on a change in pull request #10686:
URL: https://github.com/apache/tvm/pull/10686#discussion_r831382689
##########
File path: tests/python/unittest/test_auto_scheduler_feature.py
##########
@@ -220,12 +220,15 @@ def tir_matmul(
def test_primfunc():
- features = auto_scheduler.feature.named_features_from_primfunc(tir_matmul)
Review comment:
Great, seems our interpretations aligns with each other. I agree that
having a test that thread through lowered is useful if we bring in future
usecases that applies feature extractor on lowered code.
To summarize the recommendation:
- Keep the original `test_primfunc`(perhaps rename to
`test_prim_func_before_lowering` to clairfy the intend)
- This was the original intended behavior of the extractor and
auto-scheduler only relied on this behavior.
- Add a new test `test_primfunc_lowered_with_let`
- I understand that there is an exploration effort to try out this and
possibly followup features that depends on this. Although auto-scheduler do not
yet rely on this behavior, so it would be good to keep it separate.
- Like you said it is more like an integration tests(but i agree in this
case it is OK to keep it here)
- (Optional): add a unit-test testcase with explicitly constructed
tvmscript that covers the `lowered_let`. The main rationale is that the intend
of the unit-test case would be clear and it becomes easier to debug in facing
regression, but I feel in this case it can be optional.
--
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]
