[AOT] Added a test for detecting output size post MLF export #13655
Conversation
|
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 |
python/tvm/testing/aot.py
Outdated
| data_len_var = actual_data_name + "_len" | ||
| main_file.write( | ||
| f"const size_t {data_len_var}" | ||
| f"= sizeof({actual_data_name})/sizeof({actual_data_name}[0]);\n" |
There was a problem hiding this comment.
We generate actual_data_name and data_length_var_name in the inputs/outputs already, why are we recalculating it here based on types we've defined within the AOT test harness?
We probably need to add a _LEN macro to the MLF header?
There was a problem hiding this comment.
As discussed offline, the two variables LEN and SIZE serve different purposes. SIZE will come from MLF export whereas LEN will be part of the AOT test harness.
Follow up: apache#12789 -Added a separate test to detect output size from MLF codegen -Updated test harness to detect correct size from IO arrays -MLF size was not used in aot.py because its unavailable in case of packed apis
|
@Mousius could you please take a look again after the LEN was included from AOT test harness? |
| file_list = [] | ||
| for path in (pathlib.Path(base_path) / "codegen" / "host" / "include").iterdir(): | ||
| if path.is_file(): | ||
| file_list.append(path) |
There was a problem hiding this comment.
Given we know the model_name can we not just look for tvmgen_{model_name}.h rather than looping?
There was a problem hiding this comment.
Can we also do this for the input sizes?
There was a problem hiding this comment.
I will extend the check for inputs. We could directly look for the file, but I thought that check maynot work for multiple models. But it does, so I will update that too.
There was a problem hiding this comment.
I assume this just requires looking for both headers?
There was a problem hiding this comment.
Oh, there is just one header in those cases too. Both models' sizes appear in a single header. So, need not be tested additionally.
There was a problem hiding this comment.
That sounds wrong, shouldn't there be a tvmgen_model1.h and a tvmgen_model2.h ?
There was a problem hiding this comment.
My bad. I confused this with the multi-model test which it is not. In case of multi model test, I do see two separate headers being produced.
| assert contents.count("_SIZE") == 4 | ||
| assert str(ref_output_size) in contents |
There was a problem hiding this comment.
We should probably check the _SIZE values match with the appropriate constants rather than them just appearing in the same file together?
There was a problem hiding this comment.
I tried doing that initially. Any short cuts to do that?
There was a problem hiding this comment.
Something like:
assert contents.count("_SIZE") == 4
assert f"INPUT_1_SIZE {ref_output_size}" in contents
?
There was a problem hiding this comment.
Ah ok. I misunderstood what you were asking for. This makes sense. Thanks for the help.
Change-Id: I07d470e4a8eabc75912675a56c528354c7d40981
|
LGTM! Thanks @ashutosh-arm ! |
…13655) Follow up: apache#12789 -Added a separate test to detect output size from MLF codegen
3 participants
Follow up from: #12789
-Fixed IO size map preparation in MLF export
-Added a separate test to detect output size from MLF codegen
-MLF size is not used in aot.py because its unavailable in case of packed apis
cc @Mousius