You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2020/04/22 15:41:35 UTC
[GitHub] [incubator-tvm] mbaret opened a new pull request #5409: [BYOC] Don't annotate constants
mbaret opened a new pull request #5409:
URL: https://github.com/apache/incubator-tvm/pull/5409
By annotating constants, constant values were frequently getting passed between partitions. As constant values aren't really data flow paths, it's better to not annotate them at all so they remain with the functions that consume them.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] mbaret commented on pull request #5409: [BYOC] Don't annotate constants
Posted by GitBox <gi...@apache.org>.
mbaret commented on pull request #5409:
URL: https://github.com/apache/incubator-tvm/pull/5409#issuecomment-619080360
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'. By this, I mean that the following structure:
```
input
|
begin
|
op -- const
|
end
```
while not explicitly annotating every node, should imply that the const is still in a region, just not as an input or output. If we put a begin before the const, that implies there's a data flow path into the region through the constant, which isn't strictly true. Maybe this would require a slightly modified traversal in AnnotatedRegionSet to handle this case.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] zhiics commented on pull request #5409: [BYOC] Don't annotate constants
Posted by GitBox <gi...@apache.org>.
zhiics commented on pull request #5409:
URL: https://github.com/apache/incubator-tvm/pull/5409#issuecomment-620687954
@mbaret yeah, I think that's generally good though. We can probably send an RFC to see if we would break some other things. Otherwise, we can only bake it into partitioning.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] comaniac commented on pull request #5409: [BYOC] Don't annotate constants
Posted by GitBox <gi...@apache.org>.
comaniac commented on pull request #5409:
URL: https://github.com/apache/incubator-tvm/pull/5409#issuecomment-620711882
@manupa-arm I agree with your point and I prefer A1 more because it at least maintains annotation pairs. A1 actually treats a constant node as an input variable when annotating a target. It seems reasonable to me.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] mbaret commented on pull request #5409: [BYOC] Don't annotate constants
Posted by GitBox <gi...@apache.org>.
mbaret commented on pull request #5409:
URL: https://github.com/apache/incubator-tvm/pull/5409#issuecomment-620051291
I don't think we'd need to treat constant nodes differently everywhere else. At least, in the previous version of AnnotatedRegionSet using the recursive pattern, the constants ended up in the correct region without explicit annotation. I think with the new traversal order though this no longer happens.
The main reason I'm not in favour of explicitly annotating a constant with begin/end is that it implies there is a data flow path in through the constant. We're then violating a different assumption which is that all begin/end annotations cut across data flow.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] mbaret commented on pull request #5409: [BYOC] Don't annotate constants
Posted by GitBox <gi...@apache.org>.
mbaret commented on pull request #5409:
URL: https://github.com/apache/incubator-tvm/pull/5409#issuecomment-621253051
I've implemented an alternative fix in partition_graph.cc by explicitly binding constant tuples (see [5476](https://github.com/apache/incubator-tvm/pull/5476)). This means this PR is no longer required.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] comaniac edited a comment on issue #5409: [BYOC] Don't annotate constants
Posted by GitBox <gi...@apache.org>.
comaniac edited a comment on issue #5409:
URL: https://github.com/apache/incubator-tvm/pull/5409#issuecomment-617912766
Thanks for the PR. Could you explain further about why the current solution will pass constants between partitions? AFAIK, constant nodes should be included in a subgraph if we bind the parameters.
Apart from the motivation, my understanding to PR is to create the following graph:
```
input
|
begin
|
op -- const
|
end
```
where the constant input for `op` is not annotated.
Since now we require `AnnotateTarget` to annotate every op, and `AnnotateRegionSet`, `MergeCompilerRegion` also require every op to be annotated, could you elaborate if this change works for all of them? At first glance, it might be a problem for `AnnotateRegionSet` to identify the region inputs.
Following the above concern, could you also add this case to the unit tests of other passes so that we can make sure the rest passes are covered.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] mbaret commented on issue #5409: [BYOC] Don't annotate constants
Posted by GitBox <gi...@apache.org>.
mbaret commented on issue #5409:
URL: https://github.com/apache/incubator-tvm/pull/5409#issuecomment-618290965
The motivation for this is that when you have a supported operator, such as qnn.concatenate, which takes some constant inputs, those constant nodes are annotated as 'default'. So the operator itself goes into a target partition whereas the constants remain in the default partition and are then passed to the target partition as arguments.
To see this, I can continue to run the test case through partition graph both with and without this change. Without the change:
```
def @const_test_0(%const_test_0_i0: (Tensor[(10, 10), uint8], Tensor[(10, 10), uint8]), %const_test_0_i1: (float32, float32), %const_test_0_i2: (int32, int32), Inline=1, Compiler="const_test", global_symbol="const_test_0", Primitive=1) -> Tensor[(10, 20), uint8] {
qnn.concatenate(%const_test_0_i0, %const_test_0_i1, %const_test_0_i2, 0f /* ty=float32 */, 1 /* ty=int32 */, axis=1) /* ty=Tensor[(10, 20), uint8] */
}
def @main(%a: Tensor[(10, 10), uint8], %b: Tensor[(10, 10), uint8]) -> Tensor[(10, 20), uint8] {
%0 = abs(%a) /* ty=Tensor[(10, 10), uint8] */;
%1 = (%0, %b);
%2 = (0f /* ty=float32 */, 0f /* ty=float32 */);
%3 = (1 /* ty=int32 */, 1 /* ty=int32 */);
@const_test_0(%1, %2, %3) /* ty=Tensor[(10, 20), uint8] */
}
```
With the change:
```
def @const_test_0(%const_test_0_i0: (Tensor[(10, 10), uint8], Tensor[(10, 10), uint8]), Inline=1, Compiler="const_test", global_symbol="const_test_0", Primitive=1) -> Tensor[(10, 20), uint8] {
%0 = (0f /* ty=float32 */, 0f /* ty=float32 */);
%1 = (1 /* ty=int32 */, 1 /* ty=int32 */);
qnn.concatenate(%const_test_0_i0, %0, %1, 0f /* ty=float32 */, 1 /* ty=int32 */, axis=1) /* ty=Tensor[(10, 20), uint8] */
}
def @main(%a: Tensor[(10, 10), uint8], %b: Tensor[(10, 10), uint8]) -> Tensor[(10, 20), uint8] {
%2 = abs(%a) /* ty=Tensor[(10, 10), uint8] */;
%3 = (%2, %b);
@const_test_0(%3) /* ty=Tensor[(10, 20), uint8] */
}
```
I'll add this as an explicit test case in the graph partition tests.
The reason I've chosen to just omit the annotations for constants altogether is that the requirement for MergeCompilerRegions is that all dataflow paths are annotated. As constants are just data with no flow, they can be considered as part of the operator which consumes them for the purposes of the analysis done in that pass.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] mbaret commented on pull request #5409: [BYOC] Don't annotate constants
Posted by GitBox <gi...@apache.org>.
mbaret commented on pull request #5409:
URL: https://github.com/apache/incubator-tvm/pull/5409#issuecomment-618895831
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.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] mbaret commented on pull request #5409: [BYOC] Don't annotate constants
Posted by GitBox <gi...@apache.org>.
mbaret commented on pull request #5409:
URL: https://github.com/apache/incubator-tvm/pull/5409#issuecomment-620684416
Explicitly binding constant tuples in the partitioner seems like a reasonable solution. It would involve generalizing BindParamsByName to work with tuples though (unless we wrote a custom version of that just for partition_graph).
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] comaniac commented on pull request #5409: [BYOC] Don't annotate constants
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [incubator-tvm] comaniac edited a comment on issue #5409: [BYOC] Don't annotate constants
Posted by GitBox <gi...@apache.org>.
comaniac edited a comment on issue #5409:
URL: https://github.com/apache/incubator-tvm/pull/5409#issuecomment-618537434
Ah I see your point. I missed considering that `ConstantNode` is also a "node" instead of a "var". I agree with you that constant nodes should follow the target of the consuming node and the output you illustrated makes sense to me.
On the other hand, it'd be better to maintain the statement of "annotating every node" of `AnnotateTarget` pass in my opinion. A similar case is `TupleNode`. We determine the target of tuple nodes by looking at its arguments, and still annotate it whatever it should be at the TVM or other targets. In summary, I'd prefer the following annotation generated by `AnnotateTarget`:
```
input
|
begin
|
op -- begin -- end -- const -- begin
|
end
```
Although just like you pointed out that constant node is not in dataflow path, I still prefer to simplify the process in a pass. This can not only naturally guarantee that this change will not affect the rest BYOC flow, but also make the future maintenance easier.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] comaniac commented on issue #5409: [BYOC] Don't annotate constants
Posted by GitBox <gi...@apache.org>.
comaniac commented on issue #5409:
URL: https://github.com/apache/incubator-tvm/pull/5409#issuecomment-617912766
Thanks for the PR, so the idea is to create the following graph if I understand correctly:
```
input
|
begin
|
op -- const
|
end
```
where the constant input for `op` is not annotated.
Since now we require `AnnotateTarget` to annotate every op, and `AnnotateRegionSet`, `MergeCompilerRegion` also require every op to be annotated, could you elaborate if this change works for all of them? At first glance, it might be a problem for `AnnotateRegionSet` to identify the region inputs.
Following the above concern, could you also add this case to the unit tests of other passes so that we can make sure the rest passes are covered.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] zhiics commented on pull request #5409: [BYOC] Don't annotate constants
Posted by GitBox <gi...@apache.org>.
zhiics commented on pull request #5409:
URL: https://github.com/apache/incubator-tvm/pull/5409#issuecomment-620674803
Sorry for the late followup. I think both points make sense at certain aspect. @manupa-arm I also think A1 should have been covered by binding constants. Fixing constant tuples looks like a good way to fix this problem though, and it would be helpful to other passes in Relay as well.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] manupa-arm commented on pull request #5409: [BYOC] Don't annotate constants
Posted by 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
[GitHub] [incubator-tvm] zhiics edited a comment on pull request #5409: [BYOC] Don't annotate constants
Posted by GitBox <gi...@apache.org>.
zhiics edited a comment on pull request #5409:
URL: https://github.com/apache/incubator-tvm/pull/5409#issuecomment-620687954
@mbaret yeah, I think that's generally good though. We can probably send an RFC to see if we would break some other things as if it is usually the first "pass" in build module. Otherwise, we can only bake it into partitioning.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] comaniac commented on issue #5409: [BYOC] Don't annotate constants
Posted by GitBox <gi...@apache.org>.
comaniac commented on issue #5409:
URL: https://github.com/apache/incubator-tvm/pull/5409#issuecomment-618537434
Ah I see your point. I missed considering that `ConstantNode` is also a "node" instead of a "var". I agree with you that constant nodes should follow the target of the consuming node and the output you illustrated makes sense to me. On the other hand, it'd be better to maintain the statement of "annotating every node" of `AnnotateTarget` pass. A similar case is `TupleNode`. We determine the target of tuple nodes by looking at its arguments, and still annotate it whatever it should be at the TVM or other targets. In summary, I'd prefer the following annotation generated by `AnnotateTarget`:
```
input
|
begin
|
op -- begin -- end -- const -- begin
|
end
```
instead of
```
input
|
begin
|
op -- const
|
end
```
This can naturally guarantee that this change will not affect the rest BYOC flow.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] trevor-m commented on issue #5409: [BYOC] Don't annotate constants
Posted by GitBox <gi...@apache.org>.
trevor-m commented on issue #5409:
URL: https://github.com/apache/incubator-tvm/pull/5409#issuecomment-618051867
Does this affect the end result of the partitioned subgraphs at all? Will the constant still be included in the subgraph?
----------------------------------------------------------------
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