Mousius commented on PR #12550:
URL: https://github.com/apache/tvm/pull/12550#issuecomment-1225832264
> > Which is an internal optimisation inside of ExprAllocator that hopefully
we don't remove but makes no difference to the outer behaviour of the AOT pass
and is therefore difficult to assert is behaving correctly.
>
> If that code is unreachable from AOTMainLowerer, it is fundamentally
unreachable from the program that is 'TVM', and at the end of the day TVM is
the component under test. If it is reachable, then we can test it through the
AOTMainLowerer interface.
The line itself is not unreachable, it's harder to test and ensure the
behaviour from within `AOTMainLowerer` as you're essentially testing "nothing
happened" with the `kPrimitive` attribute added. There are always pieces of
behaviour which are not explicitly user facing but are important for the
overall behaviour of the program, else you could consider the existing AOT
tests which invoke `relay.build` to be sufficient?
> I think it is reachable because we can always construct a test case with a
function marked `kPrimitive` for AOTMainLowerer. If that's the primary concern
here I'll happily add in such a test. I would caution though that the [TVM Code
Quality
Guidelines](https://github.com/apache/tvm/blob/main/docs/contribute/code_review.rst#factors-to-consider-about-code-quality)
make no commitment to 100% coverage, so I think it's important we focus our
testing effort wisely (I certainly haven't covered every corner case in the
current tests).
Yip, you can interpret "test coverage" differently, I'd rather not gamble on
whether a future change will be caught or not by the tests though 😸
--
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]
