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/09/09 10:01:38 UTC
[GitHub] [incubator-tvm] lhutton1 opened a new pull request #6430: [ConvertLayout] Use a custom layout function to decide layout based on operators attributes and arguments
lhutton1 opened a new pull request #6430:
URL: https://github.com/apache/incubator-tvm/pull/6430
Allows the user to specify a 'custom' label on the operators they wish to use a separate function to determine the layout, rather than relying on purely the operator name. One use-case example of this is were one might require a different layout for depth-wise convolution and other types of convolution. e.g. IHWO for depthwise, else OHWI.
See convert_layout.rst for a more detailed explanation.
cc @anijain2305 @manupa-arm @mbaret
----------------------------------------------------------------
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] lhutton1 commented on pull request #6430: [ConvertLayout] Use a packed function to decide layout based on operator attributes
Posted by GitBox <gi...@apache.org>.
lhutton1 commented on pull request #6430:
URL: https://github.com/apache/incubator-tvm/pull/6430#issuecomment-690617359
Yep that's correct. I think the current PR has one advantage being that a "custom" layout is explicitly defined and passed to convert layout as opposed to overriding a function that may seem unrelated to the convert layout pass as first glance. However, I suppose there is also a question as to whether we want to duplicate functionality that can already be made use of. Is there a reason the components of the operator registry were never exposed to C++?
----------------------------------------------------------------
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] anijain2305 edited a comment on pull request #6430: [ConvertLayout] Use a packed function to decide layout based on operator attributes
Posted by GitBox <gi...@apache.org>.
anijain2305 edited a comment on pull request #6430:
URL: https://github.com/apache/incubator-tvm/pull/6430#issuecomment-690504454
I think calling ConvertLayout in C++ should be ok. The thing that I don't fully understand is why do we need to set the `FTVMConvertOpLayout` in C++? Is there a way to do it in python? If there is a high level python API call, we can wrap that with `with TempOpAttr`?
----------------------------------------------------------------
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] lhutton1 commented on pull request #6430: [ConvertLayout] Use a packed function to decide layout based on operator attributes
Posted by GitBox <gi...@apache.org>.
lhutton1 commented on pull request #6430:
URL: https://github.com/apache/incubator-tvm/pull/6430#issuecomment-696743382
Thanks for the response, I will close this PR as I now have a solution for my use case that doesn't require this.
----------------------------------------------------------------
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] anijain2305 commented on pull request #6430: [ConvertLayout] Use a packed function to decide layout based on operator attributes
Posted by GitBox <gi...@apache.org>.
anijain2305 commented on pull request #6430:
URL: https://github.com/apache/incubator-tvm/pull/6430#issuecomment-690600943
Yes, that makes sense. Thanks for explaining!
Yes, if we want to go through something like `TempOpAttr`, we need that functionality in C++.
Is your current PR implementation easier or more scalable then? I think in your current PR, you can define the TVMPackedFunc counterpart in the `PreProcessModule` C++ itself?
----------------------------------------------------------------
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] tqchen commented on pull request #6430: [ConvertLayout] Use a packed function to decide layout based on operator attributes
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6430:
URL: https://github.com/apache/incubator-tvm/pull/6430#issuecomment-695828553
Sorry for the delayed reply. I think it is indeed better to make use of a single way to setup the layout rules. We might want to have a helper function to override such rule when we enter the scope, and recover when we exit.
Will let @anijain2305 manage the PR
----------------------------------------------------------------
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] anijain2305 commented on pull request #6430: [ConvertLayout] Use a packed function to decide layout based on operator attributes
Posted by GitBox <gi...@apache.org>.
anijain2305 commented on pull request #6430:
URL: https://github.com/apache/incubator-tvm/pull/6430#issuecomment-689703548
Hi @lhutton1 Thanks for the PR! I am trying to understand the need for this PR.
We can always overwrite the existing convert_op_layout for each operator. So, I don't think I fully understand that need and usage.
For example, I can use the following code to perform the same operation (see that I am using a higher level - **plevel = 11**). This will be set by the TVM user and don't need to be the part of TVM codebase (similar to your usage).
~~~
@reg.register_convert_op_layout("nn.conv2d", level=11)
def custom_layouts(op_name, attrs, args):
if op_name == "nn.conv2d":
data = args[0].checked_type
weight = args[1].checked_type
is_depthwise = is_depthwise_conv2d(data.shape,
attrs['data_layout'],
weight.shape,
attrs['kernel_layout'],
attrs['groups'])
return ['NHWC', 'IHWO'] if is_depthwise else ['NHWC', 'OHWI']
raise ValueError(f'No custom layout defined for {op_name}')
~~~
Does that make sense?
----------------------------------------------------------------
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] lhutton1 commented on pull request #6430: [ConvertLayout] Use a packed function to decide layout based on operator attributes
Posted by GitBox <gi...@apache.org>.
lhutton1 commented on pull request #6430:
URL: https://github.com/apache/incubator-tvm/pull/6430#issuecomment-696743382
Thanks for the response, I will close this PR as I now have a solution for my use case that doesn't require this.
----------------------------------------------------------------
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] tqchen commented on pull request #6430: [ConvertLayout] Use a packed function to decide layout based on operator attributes
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6430:
URL: https://github.com/apache/incubator-tvm/pull/6430#issuecomment-695828553
Sorry for the delayed reply. I think it is indeed better to make use of a single way to setup the layout rules. We might want to have a helper function to override such rule when we enter the scope, and recover when we exit.
Will let @anijain2305 manage the PR
----------------------------------------------------------------
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] lhutton1 commented on pull request #6430: [ConvertLayout] Use a packed function to decide layout based on operator attributes
Posted by GitBox <gi...@apache.org>.
lhutton1 commented on pull request #6430:
URL: https://github.com/apache/incubator-tvm/pull/6430#issuecomment-689718453
Thanks @anijain2305 that makes sense, I wasn't aware of this functionality. This would need to exist in the TVM codebase as my intention was to use this with the Arm Compute Library integration. I'll look into this further tomorrow
----------------------------------------------------------------
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] lhutton1 commented on pull request #6430: [ConvertLayout] Use a packed function to decide layout based on operator attributes
Posted by GitBox <gi...@apache.org>.
lhutton1 commented on pull request #6430:
URL: https://github.com/apache/incubator-tvm/pull/6430#issuecomment-690584984
So since convert layout is part of preprocessing the module in the optimize step before codegen (https://github.com/apache/incubator-tvm/blob/master/src/relay/backend/contrib/arm_compute_lib/codegen.cc#L342) we could do this:
```
with TempOpAttr("nn.conv2d", "FTVMConvertOpLayout", ...):
graph, lib, parmas = tvm.build(...)
```
However it's not good expect the user to know how to add this. Therefore, my plan was to set the attribute for the scope of `PreProcessModule` https://github.com/apache/incubator-tvm/blob/master/src/relay/backend/contrib/arm_compute_lib/codegen.cc#L340. Hope that makes more sense?
----------------------------------------------------------------
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] lhutton1 closed pull request #6430: [ConvertLayout] Use a packed function to decide layout based on operator attributes
Posted by GitBox <gi...@apache.org>.
lhutton1 closed pull request #6430:
URL: https://github.com/apache/incubator-tvm/pull/6430
----------------------------------------------------------------
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] lhutton1 commented on pull request #6430: [ConvertLayout] Use a packed function to decide layout based on operator attributes
Posted by GitBox <gi...@apache.org>.
lhutton1 commented on pull request #6430:
URL: https://github.com/apache/incubator-tvm/pull/6430#issuecomment-691930003
ping @tqchen when you have time :)
----------------------------------------------------------------
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] lhutton1 closed pull request #6430: [ConvertLayout] Use a packed function to decide layout based on operator attributes
Posted by GitBox <gi...@apache.org>.
lhutton1 closed pull request #6430:
URL: https://github.com/apache/incubator-tvm/pull/6430
----------------------------------------------------------------
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] anijain2305 commented on pull request #6430: [ConvertLayout] Use a packed function to decide layout based on operator attributes
Posted by GitBox <gi...@apache.org>.
anijain2305 commented on pull request #6430:
URL: https://github.com/apache/incubator-tvm/pull/6430#issuecomment-690618685
@tqchen Do you have any suggestions here?
----------------------------------------------------------------
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] lhutton1 commented on pull request #6430: [ConvertLayout] Use a packed function to decide layout based on operator attributes
Posted by GitBox <gi...@apache.org>.
lhutton1 commented on pull request #6430:
URL: https://github.com/apache/incubator-tvm/pull/6430#issuecomment-690476905
Thanks for the pointer @anijain2305. I do agree this is a better approach. I have an example working on my end for my use-case, although it seems quite messy. Setting temporary attributes i.e. `FTVMConvertOpLayout` on the C++ side of things is more difficult than from python. The reason I need to do this in C++ is because I run the convert layout pass in Arm Compute Library codegen.
I've had a go at implementing something like the `TempOpAttr` class from python in C++. This is to ensure I'm only setting this config for the Arm Compute Library codegen. Although, this involves fetching a series of packed functions (namely OpGetAttr, OpResetAttr and OpSetAttr) from `src/ir/op.cc` which doesn't sound right. I'm just wondering if it sounds like I'm completely off piste with this or whether you know of anything that could help that I've missed?
----------------------------------------------------------------
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] anijain2305 commented on pull request #6430: [ConvertLayout] Use a packed function to decide layout based on operator attributes
Posted by GitBox <gi...@apache.org>.
anijain2305 commented on pull request #6430:
URL: https://github.com/apache/incubator-tvm/pull/6430#issuecomment-690504454
I think calling ConvertLayout in C++ should be ok. The thing that I don't fully understand is why do we need to set the `FTVMConvertOpLayout` in C++? Is there a way to do it in python? If there is a high level python API call, we can wrap that with `with TempOpAttr` and that should overwrite the registry in the `with` scope.
----------------------------------------------------------------
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