Conversation
|
I didn't look yet, but it'd be useful anyways. Could you explain why this is a breaking change in the description? And, I suppose we need to add smth like BREAKING CHANGE (?) somewhere |
493638b to
773dc2f
Compare
Done. The exclamation mark serves the purpose of conveying breaking changes. |
Are we adding a feature here, or are we fixing something? |
I have thought a little about it, and I'm not sure what to call it. I'm hesitant to call it a fix because we're not fixing broken functionality (bug) -- we are adding something that simply didn't exist before. On the other hand it's also a bit of a stretch to call it a feature because the validation doesn't bring any new usable functionality. But I do lean more towards feat because it's still something new that is added. |
Here is my perspective, let me know what you think: I think we're fixing a bug because in the validate() calls, we should have been returning And, this is not a feature because we're not adding any new functionality. We're merely fixing a bug and possibly limiting our support set as a result of this conservative checks. |
Yes it also makes sense. I think my reasoning was that if the library is used as intended, will something then go wrong? From that perspective, no, it's not a bug. But if |
The size check implies a tensor size restriction to 2^31-1 bytes. Kernel configurations larger than that will no longer validate. Resolves: COMPMID-8697 Signed-off-by: Andreas Flöjt <andreas.floejt@arm.com> Change-Id: I54f73ade5cb4a0d34d831505d83d1d7ef526b5db
773dc2f to
f6ceb31
Compare
gunes-arm
left a comment
There was a problem hiding this comment.
I've only been able to check until NEGather. I'll continue.
| } | ||
| else | ||
| { | ||
| // Ignored; dynamic block is deprecated. |
There was a problem hiding this comment.
There are two implementations of this kernel: one with a statically known block shape and this one with a dynamic block shape from a tensor. The dynamic block shape implementation doesn't have a default configured shape so there's nothing to validate when output is not configured. I'd even argue that we could assert that output->total_size() != 0, but I don't know. Regardless, validate and configure for dynamic block shape were supposed to be removed in 23.08 according to their deprecation note in the API doc. To avoid problems from doing so now, I instead left a comment that this else case is deliberately ignored specifically because it's deprecated anyway.
There was a problem hiding this comment.
(The same for NEBatchToSpaceLayerKernel.)
| const size_t one_channel = 1u; | ||
| ARM_COMPUTE_RETURN_ERROR_ON(input->num_channels() != one_channel); | ||
|
|
||
| TensorShape output_shape = arm_compute::misc::shape_calculator::compute_gather_shape( |
| ARM_COMPUTE_RETURN_ERROR_ON(anchors->num_dimensions() > 2); | ||
| size_t feature_height = info.feat_height(); | ||
| size_t feature_width = info.feat_width(); | ||
| size_t num_anchors = anchors->dimension(1); |
| ARM_COMPUTE_RETURN_ERROR_ON(output->dimension(1) != 2); | ||
| } | ||
| // There is no default configure, so we expect output to be initialized. | ||
| ARM_COMPUTE_ERROR_ON(output->total_size() == 0); |
There was a problem hiding this comment.
We shouldn't throw anything from validate(). The user should get a Status when they input invalid output tensor info.
| ARM_COMPUTE_RETURN_ERROR_ON(input->num_dimensions() > 4); | ||
| ARM_COMPUTE_RETURN_ERROR_ON(block_shape_x < 1 || block_shape_y < 1); | ||
|
|
||
| TensorShape expected_output_shape = misc::shape_calculator::compute_space_to_batch_shape( |
| @@ -44,6 +45,7 @@ bool CPPUpsampleKernel::is_parallelisable() const | |||
| void CPPUpsampleKernel::configure(const ITensor *input, ITensor *output, const PadStrideInfo &info) | |||
There was a problem hiding this comment.
We should add validate() calls to some kernels, especially in CPU and call their validate functions from the callers. But, I think it should be a separate ticket. Can you create a list of kernels, such as this one, that doesn't have validate() function and create a ticket for this?
The size check implies a tensor size restriction to 2^31-1 bytes. Kernel
configurations larger than that will no longer validate.
Resolves: COMPMID-8697
Change-Id: I54f73ade5cb4a0d34d831505d83d1d7ef526b5db