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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
