[GitHub] [tvm] masahi commented on pull request #8931: [TIR] Fixed LowerThreadallreduce not remapping Store buffer var

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

masahi commented on pull request #8931:
URL: https://github.com/apache/tvm/pull/8931#issuecomment-913958728


   > I just have one question. The bug seems critical, but why didn't I encounter any unexpected error when I tried several cross-thread reduction demos (including softmax) before?
   
   @MasterJH5574 yeah this is an interesting question. I think this bug manifest only if the following two conditions are met:
   1. Reduction op should be followed by elemwise ops at topi level. This applies to softmax, but not to other reduction ops like `sum`, `mean` etc since we apparently don't fuse reduction ops with elemwise ops at Relay level.
   2. Topi reduction + elemwise ops should be followed by another elemwise ops. Softmax alone doesn't hit this error because the next compute of softmax summation is the output of softmax itself, which will not be affected by `StorageRewrite`. However, if softmax is fused by other elemwise ops, the original storage for the softmax output can become an intermediate, local storage, which can be rewritten by `StorageRewrite` pass. The test case added in this PR trigger this behavior. Originally, I found this bug when I was working on making softmax op fusbile https://github.com/apache/tvm/pull/8909.  


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