Beya2019 commented on a change in pull request #6443: URL: https://github.com/apache/incubator-tvm/pull/6443#discussion_r486734730
########## File path: python/tvm/relay/op/vision/rcnn.py ########## @@ -24,7 +24,7 @@ def roi_align(data, rois, pooled_size, spatial_scale, sample_ratio=-1, layout='N Parameters ---------- data : relay.Expr - 4-D tensor with shape [batch, channel, height, width] + 4-D tensor with shape [batch, channel, height, width] or [batch, height, width, channel] Review comment: Hi @kevinthesun and @anijain2305, This NHWC layout topi implementation for roi_align is really not currently supported for traditional TVM targets. But in relay/frontend/mxnet.py the _mx_roi_align operator is fixed to NCHW(new_attrs["layout"] = "NCHW"), that is only the nchw topi implemention process is called normally. For the special target which is only support nhwc layout, we can add extra code in mxnet.py(or other frontend py) as follows(RFC:https://tvm.apache.org/docs/dev/convert_layout.html#usage): ``` desired_layouts = {'vision.roi_align': ['NHWC', `default`]} # Convert the layout to NCHW # RemoveUnunsedFunctions is used to clean up the graph. seq = tvm.transform.Sequential([relay.transform.RemoveUnusedFunctions(), relay.transform.ConvertLayout(desired_layouts)]) with tvm.transform.PassContext(opt_level=3): mod = seq(mod) ``` So, it doesn't cause the compilation failure for the nhwc topi implementation process can not be called for traditional TVM targets. but it expanded the nhwc relay ir implement with their own backend code for the special target which is only support nhwc layout. Certainly, the nhwc layout topi implementation is to be realized sooner or later, I think it can be improved gradually in the future. ########## File path: python/tvm/relay/op/vision/rcnn.py ########## @@ -24,7 +24,7 @@ def roi_align(data, rois, pooled_size, spatial_scale, sample_ratio=-1, layout='N Parameters ---------- data : relay.Expr - 4-D tensor with shape [batch, channel, height, width] + 4-D tensor with shape [batch, channel, height, width] or [batch, height, width, channel] Review comment: @kevinthesun and @anijain2305, In addition, I would like to add one more point. In our own target, this implementation method really affect our performance for it add many layout_transpose operator(Our convolution only supports nhwc, if roi_align only supports nchw, it means that many additional layout_transform operators need to be inserted into the network), I also think really need nhwc layout implementation for this op considering performance. And this submission is needed for layout convert support whether nhwc layout realized or not. The two mentioned function are not conflicting but complementary, I also hope the nhwc layout topi implementation as soon as possible. ########## File path: python/tvm/relay/op/vision/rcnn.py ########## @@ -24,7 +24,7 @@ def roi_align(data, rois, pooled_size, spatial_scale, sample_ratio=-1, layout='N Parameters ---------- data : relay.Expr - 4-D tensor with shape [batch, channel, height, width] + 4-D tensor with shape [batch, channel, height, width] or [batch, height, width, channel] Review comment: "we need remove this constraint for relay type inference and move it to op strategy for corresponding targets" For the issue, there already exists the constraint as: `python/tvm/relay/op/strategy/cuda.py` ``` @roi_align_strategy.register(["cuda", "gpu"]) def roi_align_strategy_cuda(attrs, inputs, out_type, target): """roi_align cuda strategy""" strategy = _op.OpStrategy() strategy.add_implementation(wrap_compute_roi_align(topi.vision.rcnn.roi_align_nchw), wrap_topi_schedule(topi.cuda.schedule_roi_align), name="roi_align_nchw.cuda") return strategy ``` and `python/tvm/relay/op/strategy/x86.py` ``` @roi_align_strategy.register("cpu") def roi_align_strategy_cpu(attrs, inputs, out_type, target): """roi_align x86 strategy""" strategy = _op.OpStrategy() strategy.add_implementation(wrap_compute_roi_align(topi.x86.roi_align_nchw), wrap_topi_schedule(topi.generic.schedule_roi_align), name="roi_align.x86") return strategy ``` In future, we just need to realize the topi.vision.rcnn.roi_align_nhwc and add the constraint(call topi.vision.rcnn.roi_align_nhwc) to the special target. And I will realize this job as soon as possible. ########## File path: python/tvm/relay/op/vision/rcnn.py ########## @@ -24,7 +24,7 @@ def roi_align(data, rois, pooled_size, spatial_scale, sample_ratio=-1, layout='N Parameters ---------- data : relay.Expr - 4-D tensor with shape [batch, channel, height, width] + 4-D tensor with shape [batch, channel, height, width] or [batch, height, width, channel] Review comment: @kevinthesun "we need remove this constraint for relay type inference and move it to op strategy for corresponding targets" For the issue, there already exists the constraint as: `python/tvm/relay/op/strategy/cuda.py` ``` @roi_align_strategy.register(["cuda", "gpu"]) def roi_align_strategy_cuda(attrs, inputs, out_type, target): """roi_align cuda strategy""" strategy = _op.OpStrategy() strategy.add_implementation(wrap_compute_roi_align(topi.vision.rcnn.roi_align_nchw), wrap_topi_schedule(topi.cuda.schedule_roi_align), name="roi_align_nchw.cuda") return strategy ``` and `python/tvm/relay/op/strategy/x86.py` ``` @roi_align_strategy.register("cpu") def roi_align_strategy_cpu(attrs, inputs, out_type, target): """roi_align x86 strategy""" strategy = _op.OpStrategy() strategy.add_implementation(wrap_compute_roi_align(topi.x86.roi_align_nchw), wrap_topi_schedule(topi.generic.schedule_roi_align), name="roi_align.x86") return strategy ``` In future, we just need to realize the topi.vision.rcnn.roi_align_nhwc and add the constraint(call topi.vision.rcnn.roi_align_nhwc) to the special target. And I will realize this job as soon as possible. ---------------------------------------------------------------- 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