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

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

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

   > > I'm assuming this is mostly a move and a clean up with additional test coverage?
   > 
   > Yeah that's right, we were finding the the AOT codegen had become unmanageably complex and had very poor testing coverage so it was difficult to add features to. This is a first attempt at bringing it back from the brink :) There's some more context on where this is headed in the tracking issue.
   > 
   
   Makes sense, I reviewed it in this spirit rather than a complete rewrite 😸 
   
   > > It'd be good to split out the `ExprAllocator` pass and test it independently
   > 
   > I did consider this, but sided against it on the basis that `ExprAllocator` is an implementation detail of `AOTMainLowerer`. We should still be able to test all the behaviours implemented by `ExprAllocator` through the stable interface of `AOTMainLowerer`.
   > 
   > I accept there are some conflicting testing philosophies around this though (I think BDD vs. TDD being the relevant one here), and so am open to other views.
   
   In either the BDD or the TDD case you'd have tests first so no code would appear without a test to cover it. I don't think that's the case here as the tests are retro-fitted to the existing code?  If written tests first, I would expect to follow the method you've described by starting with tests for `AOTMainLowerer` and then write tests for the internal units where I can't test them as part of the higher level interface. 
   
   I don't think you can test all the behaviours of `ExprAllocator` through `AOTMainLowerer` due to cases such as:
   https://github.com/apache/tvm/pull/12550/files#diff-fd79a3a236f19a9f85e5045e7c75ed47a09ad7d698042114a93f4f74dd17c75eR99-R101
   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.
   
   The [TVM Code Quality Guidelines](https://github.com/apache/tvm/blob/main/docs/contribute/code_review.rst#factors-to-consider-about-code-quality) philosophy is that we should ensure test coverage, implying we should test these cases and also error cases which are equally part of the `Pass`es contract. 


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