kevinthesun commented on a change in pull request #6443: URL: https://github.com/apache/incubator-tvm/pull/6443#discussion_r486803979
########## 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: OK. One thing to note is that on relay level NHWC roi_align will fail during type inference since we do check NCHW layout: https://github.com/apache/incubator-tvm/blob/master/src/relay/op/vision/rcnn_op.cc#L46 If we would like to allow NHWC roi_align even on relay level, we might want to remove this check and move it to op strategy for corresponding targets, since we don't have implementation for NHWC and usually layout for roi_align is not an issue in end to end inference for these general purpose targets. ########## 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: OK. One thing to note is that on relay level we do check NCHW layout for roi_align: https://github.com/apache/incubator-tvm/blob/master/src/relay/op/vision/rcnn_op.cc#L46 If we would like to allow NHWC roi_align, we might want to remove this contraint for relay type inference and move it to op strategy for corresponding targets, since we don't have implementation for NHWC and usually layout for roi_align is not an issue in end to end inference for these general purpose targets. ########## 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: OK. One thing to note is that on relay level we do check NCHW layout for roi_align: https://github.com/apache/incubator-tvm/blob/master/src/relay/op/vision/rcnn_op.cc#L46 If we would like to allow NHWC roi_align, we might want to remove this constraint for relay type inference and move it to op strategy for corresponding targets, since we don't have implementation for NHWC and usually layout for roi_align is not an issue in end to end inference for these general purpose targets. ########## 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: Today we don't check roi_align layout in op strategy since we do the check in relay infer type level. If we want to allow NHWC in relay level, we need to remove that constraint in ROIAlignRel and move it here. Silently selecting nchw implementation and fail in later stage(even runtime) is not great. ---------------------------------------------------------------- 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