comaniac commented on pull request #5409:
URL: https://github.com/apache/incubator-tvm/pull/5409#issuecomment-619198333
> I'm concerned that if we enforce annotation of individual constant nodes,
MergeCompilerRegions will become a necessary pass to run. Otherwise we'll
generate partitions just containing a ConstantNode.
It seems to me that if someone runs `AnnotateTarget`, there's no reason for
her to skip `MergeCompilerRegions`. If she wrote her own annotate pass, then
those cases should be handled by herself. Originally I was even thinking to put
`AnnotateTarget` and `MergeCompilerRegions` in the same pass, but we have
consented to have separate passes so that we can have higher flexibility for
the merging algorithm.
> It looks like the refactor to AnnotatedRegionSet has broken this 'fix'. I
think the important property to maintain is that 'every node should belong to a
region' rather than 'every node should be annotated'.
I agree with you that every node should belong to a region is an important
property we should maintain in `AnnotateRegionSet`. I'm just concerned about
treating `ConstantNode` as a special node. In this case, we have to be aware of
`ConstantNode` everywhere in this infra, or we will easily introduce bugs when
improving one of them otherwise. I'm hoping that we could deal with the
constant nodes in `AnnotateTarget` pass and let other passes treat constant
nodes as other nodes. For example like you have illustrated, this graph
requires `AnnotateRegionSet` to deal with consant nodes:
```
input
|
begin
|
op -- const
|
end
```
but for this graph, `AnnotateRegionSet` can treat `const` as a normal op:
```
input
|
begin
|
op -- begin -- end -- const -- begin
|
end
```
----------------------------------------------------------------
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
