Skip to content

Conversation

@Ever-Kid
Copy link
Contributor

default value for index_dtype_override is null, so it would be better if the registry block of te.CreatePrimFunc could check args size.

@tvm-bot
Copy link
Collaborator

tvm-bot commented Jan 19, 2023

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

Generated by tvm-bot

@Ever-Kid
Copy link
Contributor Author

cc. @vinx13

@Hzfengsy
Copy link
Member

Could you please add a regression test?

@Ever-Kid
Copy link
Contributor Author

Could you please add a regression test?

Sure, I guess I should test te.CreatePrimFunc by runtime::Registry. Any hints where to put this testcase, like tests/cpp/data_type_rewriter_test.cc, tests/cpp/tensor_test.cc or others ?

@Hzfengsy
Copy link
Member

Hzfengsy commented Jan 19, 2023

After looking at the codebase again, it would be great if we can ensure this PackedFunc has two args. Another approach is adding ICHECK_EQ(args.size(), 2).

Keeping PackedFunction stable and minimal will benefit the registry system and user experience. If you want to call it from cpp side, we have API here, and no need to call PackedFunction

Also cc @junrushao

@junrushao
Copy link
Member

That sounds reasonable to me :-)

@tqchen
Copy link
Member

tqchen commented Jan 21, 2023

sorry i approved pre-maturely, after seeing @Hzfengsy 's comment, i agree with the accessment

@Ever-Kid
Copy link
Contributor Author

I followed @Hzfengsy 's suggestion. And since no need to call PackedFunc, also testcases like test_tensor_layout_attr in tests/python/unittest/test_te_create_primfunc.py are quite inclusive, so I added no test.
Any other suggestions would be welcomed~ 😄

@Hzfengsy Hzfengsy merged commit 558c994 into apache:main Jan 31, 2023
@Ever-Kid Ever-Kid deleted the tir_registry_fix branch February 1, 2023 01:47
fzi-peccia pushed a commit to fzi-peccia/tvm that referenced this pull request Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants