[GitHub] [tvm] areusch commented on pull request #12550: [AOT] Add AOTLowerMain pass to lower a Relay main into TIR

[GitBox <gi...@apache.org>]

areusch commented on PR #12550:
URL: https://github.com/apache/tvm/pull/12550#issuecomment-1227617729

   >>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 😸
   
   @Mousius regarding test coverage: generally I'm supportive of having more test coverage, but i'm not sure in this case it's necessary to block the PR over it; in this case, i think the regression is that we'd visit more of the main function than we'd need to in building `expr_storage_map_`. to me this falls a bit more on the "perfect" side of the don't-let-perfect-be-the-enemy-of-good argument. is there another kind of regression you can see here that is hard to test without explicitly testing ExprAllocator standalone?


-- 
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: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org