Lunderberg commented on PR #16955: URL: https://github.com/apache/tvm/pull/16955#issuecomment-2087070331
> Having view operation can in general cause problems mainly because most ops(including generated and external ones) assumes `elem_offset = 0` So long as the operations assert that the element offset is zero when this assumption is being made, this makes sense. This is what we do in `MakePackedAPI` for PrimFuncs that require the `elem_offset` to be zero. For external operations that accept an aligned pointer to data, I like your suggestion of ensuring that we provide aligned data. My plan was to only introduce views that maintain the same alignment that is provided by existing allocations. For external operations that accept a `NDArray` and assume the offset is zero without validating that assumption, I think we should view this as a bug in those external operations. If we know specific cases that ignore the offset, we can definitely insert alignment operators to provide aligned buffers. I wouldn't want to add it for every single external operation, though, because at some point we need to trust that functions accept the arguments that they say are accepted. > * Add a `R.memory.ensure_compact` operation, which can potentially results in a copy for backends that do not have direct memory ptr access, but can potentially do ptr editing for backends that support them (this would need a target dependent lowering) To be clear, do you mean `R.memory.ensure_aligned` instead of `R.memory.ensure_compact`? This has come up a few times in this discussion, and I want to make sure that we are discussing the same thing. The view operation cannot be applied to a strided `NDArray`, and its output is always compact. Having the dedicated operation for it would also work well for dynamically-shaped arguments. In those cases, we wouldn't know until runtime whether the operation requires a copy or not in order to provide an aligned argument. > Enable special ops that handle inputs which can come a view, likely only LoRA ones, which can inline view operations into ops. I agree with aiming to have views be fused with later operations where possible, though I'd add that this is not LoRA-specific functionality. Anywhere that `CombineParallelMatmul` can be used to improve the matmul performance, a view into the result can be used to avoid an unnecessary copy from the output. > * `R.view` perhaps should be renamed as `R.memory.view`, this is a more advanced operator that contains certain assumptions and likely not something we want to advertise genrally for now. I think the only assumption it makes is that a platform supports casting of pointers. Regarding names, I agree that `R.memory.view` is a better name for it, and will update the PR. -- 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]
