Lunderberg edited a comment on pull request #42:
URL: https://github.com/apache/tvm-rfcs/pull/42#issuecomment-963314748


   A slight modification to the example of using a helper function to simplify 
implementation of the visitors for `BufferStore` and `BufferLoad`.  Use of 
`CopyOnWrite` to pass a non-const reference to the helper function would cause 
unnecessary copies in the case where no change is necessary.  The copy in 
`CopyOnWrite` occurs when `CopyOnWrite()` is called, not when the write itself 
happens.  Therefore, the helper function should be rewritten in order to avoid 
this copy.  This doesn't change the conclusion, but would change the 
recommended way to write the helper function.
   
   <details>
       <summary>BufferLoad/BufferStore, using templated helper 
function</summary>
       
   ```c++
   Stmt VisitStmt_(const BufferStoreNode* op) final {
     return 
ModifyBuffer(Downcast<BufferStore>(StmtExprMutator::VisitStmt_(op)));
   }
   
   PrimExpr VisitStmt_(const BufferLoadNode* op) final {
     return ModifyBuffer(Downcast<BufferLoad>(StmtExprMutator::VisitExpr_(op)));
   }
   
   template<typename Node>
   Node ModifyBuffer(Node node) {
     auto it = buffer_map_.find(node->buffer);
     if (it != buffer_map_.end()) {
       auto writer = node.CopyOnWrite();
       writer->buffer = it->second;
     }
   
     return node;
   }
   ```
   </details>
   
   (I think the original helper function could still work if 
`obj.CopyOnWrite()->field` returned a proxy object that was convertible to the 
underlying type, and whose `operator=` performed the copy itself.  However, 
that's probably more trouble than it's worth.)


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


Reply via email to