[GitHub] [incubator-tvm] manupa-arm commented on pull request #5409: [BYOC] Don't annotate constants

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

manupa-arm commented on pull request #5409:
URL: https://github.com/apache/incubator-tvm/pull/5409#issuecomment-620500976


   I guess annotating backend-independent nodes (such as ConstantNodes, TupleGetItem, Tuple, etc) are not trivial and they are special cased. Some (e.g., TupleGetItem node) should look at parents while others (ConstantNode) should look at children. Looking at children, is quite tricky with current infrastructure (correct me, If I am wrong).
   
   Therefore, IMHO I would prefer the current A1) : 
   
   input
     |
   begin
     |
    op -- begin -- const
     |
    end
   
   OR the proposed A2) :
   
   input
     |
   begin
     |
    op -- const
     |
    end
   
   I fail to see the value addition of annotating constant nodes as they should strictly belong to the region as the op and should not be considered to be merged with other regions. In that light A2 seems appropriate. 
   
   However, the current status A1 (though it has an unnecessary boundary) should work in principle as we bind constants (@zhiics ) if they are a input to a region. But I guess, it might fail to recognize constant tuples. We could fix that as an alternative solution. 
   
   Additionally, @comaniac 's proposal makes it compulsory to run MergeCompilerRegions to avoid creating primitive functions with just a constant node in it. Since we do not have control over the output region size of MergeCompilerRegions (yet), we might not want to annotate constants (yet).
   


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