[GitHub] [tvm] giuseros commented on pull request #8096: Decoupling AOT from graph memory planner
giuseros commented on pull request #8096: URL: https://github.com/apache/tvm/pull/8096#issuecomment-869880637 -- 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-unsubscr...@tvm.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [tvm] giuseros commented on pull request #8096: Decoupling AOT from graph memory planner
giuseros commented on pull request #8096: URL: https://github.com/apache/tvm/pull/8096#issuecomment-870512072 Hi @jroesch, The solution I picked is to use the common `StorageInfo` so that later on we can have a PR to move from SoA to AoS for all the users of that structure (i.e., all the memory planning algorithms). Thanks, Giuseppe -- 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-unsubscr...@tvm.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [tvm] giuseros commented on pull request #8096: Decoupling AOT from graph memory planner
giuseros commented on pull request #8096: URL: https://github.com/apache/tvm/pull/8096#issuecomment-869972638 Hi @areusch , I can reuse that in theory, but `TempStorageInfo` is only a local structure used to have on-demand memory allocation, i.e., I don't need to pass the `TempStorageInfo` around. This was the reason you suggested to not use struct of arrays here: https://github.com/apache/tvm/pull/8096#discussion_r638171243. I personally agreed with your comment and would prefer the way it is (i.e., array of structs), because using a struct of arrays makes the code less readable. However, if you guys feel strongly about reusing the `StorageInfo` struct of arrays, I can surely do it -- 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-unsubscr...@tvm.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [tvm] giuseros commented on pull request #8096: Decoupling AOT from graph memory planner
giuseros commented on pull request #8096: URL: https://github.com/apache/tvm/pull/8096#issuecomment-869880637 Hi @manupa-arm , @jroesch , @areusch , I just rebased and had to change `StorageInfo` to `TempStorageInfo` because of a conflict. Please, let me know if this is OK for you! Thanks, Giuseppe -- 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-unsubscr...@tvm.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [tvm] giuseros commented on pull request #8096: Decoupling AOT from graph memory planner
giuseros commented on pull request #8096: URL: https://github.com/apache/tvm/pull/8096#issuecomment-867704948 A friendly ping @manupa-arm , @jroesch , @areusch ! -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [tvm] giuseros commented on pull request #8096: Decoupling AOT from graph memory planner
giuseros commented on pull request #8096: URL: https://github.com/apache/tvm/pull/8096#issuecomment-866374368 Hi @manupa-arm , @areusch , Quick update on this. I added a unit test and slightly changed the pass to address some issues. This should be good enough to be reviewed. Thanks, Giuseppe -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [tvm] giuseros commented on pull request #8096: Decoupling AOT from graph memory planner
giuseros commented on pull request #8096: URL: https://github.com/apache/tvm/pull/8096#issuecomment-865367080 Hi @manupa-arm , @areusch , Here is a second attempt to resolve this. I basically applied what we discussed. I also cleaned a bit the TIR produced by removing all the `tir::Evaluate(0)` . Please, let me know what you guys think! -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [tvm] giuseros commented on pull request #8096: Decoupling AOT from graph memory planner
giuseros commented on pull request #8096: URL: https://github.com/apache/tvm/pull/8096#issuecomment-865367080 Hi @manupa-arm , @areusch , Here is a second attempt to resolve this. I basically applied what we discussed. I also cleaned a bit the TIR produced by removing all the `tir::Evaluate(0)` . Please, let me know what you guys think! -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [tvm] giuseros commented on pull request #8096: Decoupling AOT from graph memory planner
giuseros commented on pull request #8096: URL: https://github.com/apache/tvm/pull/8096#issuecomment-860715052 Hi @areusch , After some thinking we came up with a different solution. The way we are doing things now is the following: pass-a) Compose the `main_func` with `tvm_set_struct`, i.e., codegen ready pass-b) Storage rewrite modified to take care of the structs pass-c) tvm::build A possible alternative can be the following: pass-a2) Compose the `main_func` without `tvm_set_struct`. This means that the packed calls will receive raw pointers. This in turn means that the `main_func` in TIR is not ready to be code generated pass-b2) Storage rewrite without any change. This is possible now since we are not using `tvm_set_struct` yet pass-c2) transform the packed calls inputs by using `tvm_set_struct`. We can actually avoid this pass if we are using unpacked signatures. With pipilene-2 we can leave `StorageRewrite` unchanged and still get away with the issues that `tvm_set_struct` gives us. What do you think? -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [tvm] giuseros commented on pull request #8096: Decoupling AOT from graph memory planner
giuseros commented on pull request #8096: URL: https://github.com/apache/tvm/pull/8096#issuecomment-859519657 Hi @areusch, Thanks for your reply. I think what you are saying is can be summarized in: a) The StorageRewrite pass does not solve our problem because it can still touch the output buffers (which are parameters of the `tir PrimFunc`) b) The solution that we have put in place in `StorageRewrite` does not address the reference counting in an general way. About a), I disagree. `StorageRewrite` only rewrites `tir.Allocate` nodes, and the output is not allocated in our main runner function. This is why if you run mobilenet quantized with the `StorageRewrite` pass, it works (and it doesn't work with Graph Memory Planning). So `StorageRewrite` is a perfectly good candidate to solve the issue we have in #8062 . The problem with `StorageRewrite `is that it doesn't handle `tvm_struct_set` correctly, and this brings us to point b) About b), I agree. The solution we are proposing is not general, but any general solution can be built on top of it. As you said, we are trying to solve a specific case, but when you tackle the general case, the code I wrote is still valid. Especially in a TIR based world (which is the aim of TensorIR, afaiu) I think we will want (at some point) to extend this solution instead of accepting that `StorageRewrite` does not work with some TIR built-ins. In other words, our solution is not tailored to AOT at all. It is a partial solution to a generic problem that (only) AOT is hitting. It cannot break anything in the future, because either the user is doing what AOT is doing, and it will work. For more edge cases, `StorageRewrite` won't work as it didn't use to work before. Bear also in mind that until we have a static memory planner, without this solution AOT is not usable. If you want, we can try to write a test that stresses the problem we are trying to solve with the change in `StorageRewrite`. What do you think? -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [tvm] giuseros commented on pull request #8096: Decoupling AOT from graph memory planner
giuseros commented on pull request #8096: URL: https://github.com/apache/tvm/pull/8096#issuecomment-858730951 Hi @areusch , Any more thoughts on this? Thanks, Giuseppe -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [tvm] giuseros commented on pull request #8096: Decoupling AOT from graph memory planner
giuseros commented on pull request #8096: URL: https://github.com/apache/tvm/pull/8096#issuecomment-856279409 Hi @areusch , I applied your changes. * About the storage rewrite changes, I tried to explain more in the code. It's about the fact that in order to call a function, you need to set a struct. The rewriter focuses on the struct (marking them conflicting) but the real allocations are lost (and get reused). If you have more questions, feel free to ask. * The point about testing is that it's very useful for us to test a quantized mobilenet, and coming up with the edge situation that that network is testing is non-trivial. So I saw having that network test as a two-bird-one-stone situation. -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [tvm] giuseros commented on pull request #8096: Decoupling AOT from graph memory planner
giuseros commented on pull request #8096: URL: https://github.com/apache/tvm/pull/8096#issuecomment-856117671 Done! Thanks @mehrdadh -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [tvm] giuseros commented on pull request #8096: Decoupling AOT from graph memory planner
giuseros commented on pull request #8096: URL: https://github.com/apache/tvm/pull/8096#issuecomment-854058578 Hi @areusch , @manupa-arm , I was wondering if there was anything else you may want me to address on this PR. Thanks! -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [tvm] giuseros commented on pull request #8096: Decoupling AOT from graph memory planner
giuseros commented on pull request #8096: URL: https://github.com/apache/tvm/pull/8096#issuecomment-854058578 Hi @areusch , @manupa-arm , I was wondering if there was anything else you may want me to address on this PR. Thanks! -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [tvm] giuseros commented on pull request #8096: Decoupling AOT from graph memory planner
giuseros commented on pull request #8096:
URL: https://github.com/apache/tvm/pull/8096#issuecomment-848653432
Hi @areusch , @manupa-arm , @mehrdadh ,
Quick update on this. We found an issue in the storage rewrite pass. The
problem is due to the fact that the packed functions accept TVMValues and not
bare pointers. This means that in this TIR:
```
let sid_5_value_1: handle = @tir.tvm_stack_alloca("array", 1,
dtype=handle)
@tir.tvm_struct_set(sid_5_value_1, 0, 1, sid_5, dtype=handle)
let sid_4_value: handle = @tir.tvm_stack_alloca("array", 1, dtype=handle)
@tir.tvm_struct_set(sid_4_value, 0, 1, sid_4, dtype=handle)
{
@tir.tvm_call_cpacked("fused_transpose", sid_5_value_1, sid_4_value,
dtype=int32)
}
```
The rewriter is able to identify `sid_5_value_1` and `sid_4_value` as both
live variables, and they are not overridden. But it fails to see that those
variable relate to `sid_5` and `sid_4` (respectively).
Early on, in the calls:
```
@tir.tvm_struct_set(sid_5_value_1, 0, 1, sid_5, dtype=handle)
@tir.tvm_struct_set(sid_4_value, 0, 1, sid_4, dtype=handle)
```
The variables `sid_5` and `sid_4` are treated as separate variables (that
are NOT live at the same time) and hence get overridden. In the last commit I
explicitly create a translation table between the TVMValues and the
allocations. This seems to fix the issue. I have added also a further test
case `test_transpose` (disabling the op-fusion in Relay) to test this scenario.
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [tvm] giuseros commented on pull request #8096: Decoupling AOT from graph memory planner
giuseros commented on pull request #8096: URL: https://github.com/apache/tvm/pull/8096#issuecomment-846021337 @manupa-arm I applied your comments. Sorry, this was a draft that I quickly turned into a PR, hence the in-elegance of the code. Please have another look and let me know. -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [tvm] giuseros commented on pull request #8096: Decoupling AOT from graph memory planner
giuseros commented on pull request #8096: URL: https://github.com/apache/tvm/pull/8096#issuecomment-845244133 cc: @manupa-arm @MatthewARM @mehrdadh @areusch -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
