[GitHub] [incubator-tvm] ANSHUMAN87 edited a comment on pull request #6889: [TOPI] sparse_dense Op sparse_data input added

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

ANSHUMAN87 edited a comment on pull request #6889:
URL: https://github.com/apache/incubator-tvm/pull/6889#issuecomment-724198770


   > I'm not sure that the correct course of action is to add a flag to `sparse_dense` to support AB^T with B sparse. This makes all the implementations of `sparse_dense` confusing because they now have to check this flag and use a compute/schedule depending on if it is enabled or not. I'd instead prefer to make a new op called `dense_sparse` that does AB^T with B sparse.
   > 
   > Alternatively, I don't really see a reason for supporting AB^T with B sparse directly. Instead, when we convert from a frontend to tvm, we can just insert the correct transposes. In a lot of ways this is a better choice because we do not need to reimplement the same operators and the operators prefer the data to be in this format. I think this is the best choice.
   
   Thanks @tkonolige for response. I also had similar dilemma at the front. But later i resolved to re-utilize the existing Op, in order to enable reuse of small portion of code and it bind the concept of Op together too. But i am in favor of 'dense_sparse' Op as well(But that would be the name for existing Op i think :slightly_smiling_face: ). However i think we do not have much impact on the schedule side, in between these 2 flavor of Ops.
   
   But the utilizing Op with transpose, i felt we are adding additional overhead every-time may not be much in smaller matrix. 
   Please let me know your thoughts on this. 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:
users@infra.apache.org