[GitHub] [tvm-rfcs] Lunderberg commented on pull request #42: [TIR] Adding BufferPointer node, used by BufferLoad and BufferStore

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

Lunderberg commented on pull request #42:
URL: https://github.com/apache/tvm-rfcs/pull/42#issuecomment-957753939


   Thank you for the feedback.  Summarizing, it seems the major issues are as follows.  I have some follow-up questions for the different points, listed in the sub-bullets.
   
   * Calling the new data structure `BufferPointer` brings in lots of C assumptions of pointer handling, which could either confuse users if those assumptions aren't met, or would significantly complicate compile-time analysis if those assumptions are met.
     * Would renaming it to `BufferAccess` address these concerns, per @vinx13's [suggestion here](https://github.com/apache/tvm/pull/9391#pullrequestreview-794808362), by avoiding the comparison to C-style pointers?
   
   * Having the new data structure subclass from `PrimExpr` means that it could be used outside of just `BufferLoad` or `BufferStore`, and there's no indication at the site of the `StmtExprMutator` subclass that it should only return a specific data structure.
      * What if we add another functor, similar to the existing `ExprMutator` and `StmtMutator`, which visits objects whose mutated type must be identical to the original type?  That is, the method signature would be `BufferAccess VisitObj_(BufferAccessNode* node)`.
      * This would also be applicable to other cases that aren't currently handled, such as the `Buffer` object itself.  As an example, currently a buffer's shape may be defined in terms of a `tir::Var`.  However, `tvm::tir::Substitute` doesn't touch the buffer object, and so that variable in the buffer's shape doesn't get replaced.
   
   * Any change to TIR is a breaking change, and therefore should only be done when expanding capabilities.  Ease of development alone isn't sufficient justification for changing the TIR.
     * For my own understanding, what level of backwards compatibility does TVM aim for?  I wasn't able to find specific documentation on it.  The current implementation wouldn't satisfy any of the conditions below, but could with small modifications satisfy condition (a).  Condition (b) would be harder to meet, mostly due to the direct access of TIR node member variables, but might be possible with dummy classes and implicit type conversions.
       1. Source compatibility with external Python, but external C++ code may require updates to remain compatible.
       2. Source compatibility with external Python and C++, but external C++ code may require recompilation.
       3. Binary compatibility with external code, with new versions of `libtvm.so` usable as a drop-in replacement without recompilation.
       4. Bug-for-bug compatibility with previous versions.
     * This RFC was intended to make the proposed [RFC#0039](https://github.com/apache/tvm-rfcs/pull/39), which among other changes extends the use of `BufferLoad`/`BufferStore` and deprecates `Load`/`Store`, be simpler to implement.  Would this TIR change be more acceptable as part of a larger change, adding functionality and impacting similar TIR nodes?


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