[TVMSCRIPT] Fix printing of rank 0 buffer access #8215
Conversation
Also improve error messages and fix min/max/Select.
tkonolige
force-pushed
the
rank0_tvmscript_printer
branch
from
c116caf
to
4474c97
Compare
| @@ -180,6 +185,11 @@ def opaque_axis(begin, end, span): | |||
| return get_axis(begin, end, "opaque", span) | |||
|
|
|||
|
|
|||
| @register | |||
| def Select(cond, if_body, else_body, span): # pylint: disable=invalid-name | |||
There was a problem hiding this comment.
Could you please explain why you use capital S here?
There was a problem hiding this comment.
I'm using the capital S because it matches the definition in tir (tir.Select). The reason we do this is so that people's editors do not complain about undefined functions and so that go to definition still works.
There was a problem hiding this comment.
I recall that pylint would complain unexpected-keyword-arg if arguments mismatch occurs between TVM script and TIR node creation, for example:
tir.if_then_else(1 <= h and h < 15 and 1 <= w and w < 15,
A[n, h - 1, w - 1, i, nn, ii],
tir.float16(0),
dtype="float16")
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
pylint(unexpected-keyword-arg): Unexpected keyword argument 'dtype' in function callThere was a problem hiding this comment.
Thanks for the explanation. But I still personally prefer select since most of the existing Python APIs are using lower case (e.g. tir.min, tir. if_then_else)
On the other side, Select looks OK for me if you still persist on it.
There was a problem hiding this comment.
@tqchen What do you think?
There was a problem hiding this comment.
I think we should stick with lower cases “tir.select”, given uncased ones are used as a convention (so far) in tvm script.
On the other hand, we should think about addressing pylint complaints about undefined functions and mismatched arguments more systematically
There was a problem hiding this comment.
Thanks a lot @tkonolige
LGTM, some nits:
- Capital S in Select as @Hzfengsy pointed out.
- Print in test cases
|
|
||
| def test_rank0_blocks(): | ||
| func = rank0_block | ||
| print(tvm.script.asscript(func, True)) |
There was a problem hiding this comment.
Is it necessary to print here?
Perhaps you want to check the output with string and assert?
There was a problem hiding this comment.
Totally forgot to remove the print statements. Done.
There was a problem hiding this comment.
@junrushao1994 I am going to merge since @Hzfengsy approved, feel free to follow up if you want more changes.
* [TVMSCRIPT] Fix printing of rank 0 buffer access Also improve error messages and fix min/max/Select. * fixes * return fix * remove print
* [TVMSCRIPT] Fix printing of rank 0 buffer access Also improve error messages and fix min/max/Select. * fixes * return fix * remove print
I also improved error messages and fixed min/max/Select.
@junrushao1994 @Hzfengsy @vinx13 @MasterJH5574 @tqchen