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

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

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

   Apologies, I was away last week, so just caught up with the discussion here. 
   
   @mbaret I'm a bit confused by the addition of `CreateStorage` - you added handling (and testing) of nested functions, but we wouldn't reach that bit of code from `AOTLowerMain` since it doesn't support nested functions? It's not entirely clear to me where this came from. 
   
   My opinion regarding to the testing has not changed - I agree with @mbaret in general that we shouldn't test implementation details, but I'd test `ExprAllocator` in this case since besides being an implementation detail it is a chunky pass with complex logic. However, I don't perceive it as a fundamental issue, so I have no intention to block this PR. I also don't think the "ensure test coverage of the new feature" of the code quality guidelines is particularly helpful here since it is not defined what "test coverage" means. So I'd approve this PR once I'll understand better what is going on with the `CreateStorage` (since clearly I'm missing something).


-- 
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