From b6936e71cbd95187a6dbd507373f0db6551586f2 Mon Sep 17 00:00:00 2001 From: Hariharan Seshadri Date: Thu, 11 Apr 2019 17:44:32 -0700 Subject: [PATCH] Avoid postfix iterator increment in a loop in Slice op and some minor formatting fixes (#820) * Initial commit * Fix comment * More nit fixes --- .../providers/cpu/cpu_execution_provider.cc | 2 +- onnxruntime/core/providers/cpu/tensor/pad.cc | 18 +++--- .../core/providers/cpu/tensor/slice.cc | 47 ++++++++------- onnxruntime/core/providers/cpu/tensor/slice.h | 2 +- onnxruntime/core/providers/cpu/tensor/utils.h | 59 ++++++++++++------- 5 files changed, 71 insertions(+), 57 deletions(-) diff --git a/onnxruntime/core/providers/cpu/cpu_execution_provider.cc b/onnxruntime/core/providers/cpu/cpu_execution_provider.cc index 2a498de6b7..6f2aa018de 100644 --- a/onnxruntime/core/providers/cpu/cpu_execution_provider.cc +++ b/onnxruntime/core/providers/cpu/cpu_execution_provider.cc @@ -272,7 +272,7 @@ void RegisterOnnxOperatorKernels(KernelRegistry& kernel_registry) { kernel_registry.Register(BuildKernelCreateInfo()); kernel_registry.Register(BuildKernelCreateInfo()); kernel_registry.Register(BuildKernelCreateInfo()); - kernel_registry.Register(BuildKernelCreateInfo()); + kernel_registry.Register(BuildKernelCreateInfo()); kernel_registry.Register(BuildKernelCreateInfo()); kernel_registry.Register(BuildKernelCreateInfo()); kernel_registry.Register(BuildKernelCreateInfo()); diff --git a/onnxruntime/core/providers/cpu/tensor/pad.cc b/onnxruntime/core/providers/cpu/tensor/pad.cc index c093b4add4..896c62b830 100644 --- a/onnxruntime/core/providers/cpu/tensor/pad.cc +++ b/onnxruntime/core/providers/cpu/tensor/pad.cc @@ -48,26 +48,23 @@ static void PadAxisConstant(T* output, T constant, size_t size) { } // Flatten no padding inner most Axis, so one memcpy cover multiple Axis. -// For example, for a shape of [1,224,224,3] with padding [0,3,3,0,0,3,3,0], can be flatten as +// For example, for a shape of [1,224,224,3] with padding [0,3,3,0,0,3,3,0], can be flatten as // [1,224,224*3] with padding [0,3,3*3,0,3,3*3]. -static void FlattenInnerShape(const std::vector& input_dims, const std::vector& pads, - const std::vector& slices, std::vector& reshaped_dims) -{ +static void FlattenInnerShape(const std::vector& input_dims, const std::vector& pads, + const std::vector& slices, std::vector& reshaped_dims) { size_t dims_count = input_dims.size(); size_t inner_axis = dims_count - 1; size_t inner_size = 1; // Find all inner most dimensions that can be flattened. - do - { + do { inner_size *= input_dims[inner_axis]; if (inner_axis == 0) break; // Break on first Axis that has padding - if (!(pads[inner_axis] == 0 && pads[inner_axis + dims_count] == 0 - && slices[inner_axis] == 0 && slices[inner_axis + dims_count] == 0)) + if (!(pads[inner_axis] == 0 && pads[inner_axis + dims_count] == 0 && slices[inner_axis] == 0 && slices[inner_axis + dims_count] == 0)) break; } while (inner_axis-- > 0); @@ -79,13 +76,12 @@ static void FlattenInnerShape(const std::vector& input_dims, const std: reshaped_dims[inner_axis] = inner_size; } -static void ReshapePads(const std::vector& src_pad, size_t src_dim_count, size_t new_dim_count, +static void ReshapePads(const std::vector& src_pad, size_t src_dim_count, size_t new_dim_count, size_t inner_no_pad_size, std::vector& reshaped_pad) { - size_t inner_axis = new_dim_count - 1; std::copy(src_pad.begin(), src_pad.begin() + inner_axis, reshaped_pad.begin()); std::copy(src_pad.begin() + src_dim_count, src_pad.begin() + src_dim_count + inner_axis, reshaped_pad.begin() + new_dim_count); - + // Flatten inner axis. reshaped_pad[inner_axis] = src_pad[inner_axis] * inner_no_pad_size; reshaped_pad[inner_axis + new_dim_count] = src_pad[inner_axis + src_dim_count] * inner_no_pad_size; diff --git a/onnxruntime/core/providers/cpu/tensor/slice.cc b/onnxruntime/core/providers/cpu/tensor/slice.cc index ce831909d1..a96c5402e6 100644 --- a/onnxruntime/core/providers/cpu/tensor/slice.cc +++ b/onnxruntime/core/providers/cpu/tensor/slice.cc @@ -34,14 +34,14 @@ ADD_TYPED_SLICE_V9_OP(string); #ifndef DISABLE_CONTRIB_OPS namespace contrib { -#define ADD_TYPED_DYNAMIC_SLICE_OP(data_type) \ - ONNX_CPU_OPERATOR_TYPED_KERNEL( \ - DynamicSlice, \ - 1, \ - data_type, \ - KernelDefBuilder().TypeConstraint("T", DataTypeImpl::GetTensorType()) \ - .TypeConstraint("Tind", {DataTypeImpl::GetTensorType(), \ - DataTypeImpl::GetTensorType()}), \ +#define ADD_TYPED_DYNAMIC_SLICE_OP(data_type) \ + ONNX_CPU_OPERATOR_TYPED_KERNEL( \ + DynamicSlice, \ + 1, \ + data_type, \ + KernelDefBuilder().TypeConstraint("T", DataTypeImpl::GetTensorType()) \ + .TypeConstraint("Tind", {DataTypeImpl::GetTensorType(), \ + DataTypeImpl::GetTensorType()}), \ Slice); ADD_TYPED_DYNAMIC_SLICE_OP(uint8_t); @@ -61,14 +61,14 @@ ADD_TYPED_DYNAMIC_SLICE_OP(string); } // namespace contrib #endif -#define ADD_TYPED_SLICE_V10_OP(data_type) \ - ONNX_CPU_OPERATOR_TYPED_KERNEL( \ - Slice, \ - 10, \ - data_type, \ - KernelDefBuilder().TypeConstraint("T", DataTypeImpl::GetTensorType()) \ - .TypeConstraint("Tind", {DataTypeImpl::GetTensorType(), \ - DataTypeImpl::GetTensorType()}), \ +#define ADD_TYPED_SLICE_V10_OP(data_type) \ + ONNX_CPU_OPERATOR_TYPED_KERNEL( \ + Slice, \ + 10, \ + data_type, \ + KernelDefBuilder().TypeConstraint("T", DataTypeImpl::GetTensorType()) \ + .TypeConstraint("Tind", {DataTypeImpl::GetTensorType(), \ + DataTypeImpl::GetTensorType()}), \ Slice); ADD_TYPED_SLICE_V10_OP(uint8_t); @@ -171,7 +171,7 @@ Status SliceBase::PrepareForCompute(const std::vector& raw_starts, return Status(ONNXRUNTIME, INVALID_ARGUMENT, "'axes' has duplicates"); unique_axes.insert(axis); - // process step + // process step auto step = axis_index < raw_steps.size() ? raw_steps[axis_index] : 1; if (step == 0) return Status(ONNXRUNTIME, INVALID_ARGUMENT, "'step' value cannot be 0"); @@ -191,7 +191,7 @@ Status SliceBase::PrepareForCompute(const std::vector& raw_starts, // INT_MAX has a special meaning for end according to spec // equivalent to 'None' in numpy // it represent slicing to the end of the dimension - if (end == std::numeric_limits::max() || + if (end == std::numeric_limits::max() || end == std::numeric_limits::max()) { end = step < 0 ? -1 : input_dimensions[axis]; } @@ -202,8 +202,8 @@ Status SliceBase::PrepareForCompute(const std::vector& raw_starts, if (step < 0) end = clamp(end, int64_t{-1}, input_dimensions[axis]); else - end = clamp(end, int64_t{0}, input_dimensions[axis]); - } + end = clamp(end, int64_t{0}, input_dimensions[axis]); + } // find output dim value for this axis auto temp = static_cast(ceil(1.0 * (end - starts[axis]) / step)); @@ -290,8 +290,11 @@ Status SliceImpl(OpKernelContext* ctx, const auto* output_end = output + output_tensor.Shape().Size(); SliceIterator input_iterator(input_tensor, starts, output_dims, steps); - while (output != output_end) - *output++ = *input_iterator++; + while (output != output_end) { + *output = *input_iterator; + ++output; + ++input_iterator; + } return Status::OK(); } diff --git a/onnxruntime/core/providers/cpu/tensor/slice.h b/onnxruntime/core/providers/cpu/tensor/slice.h index 7c83285b08..5ed7459a6b 100644 --- a/onnxruntime/core/providers/cpu/tensor/slice.h +++ b/onnxruntime/core/providers/cpu/tensor/slice.h @@ -39,7 +39,7 @@ class SliceBase { std::vector& steps, std::vector& output_dims) const; - // Slice V10 & DynamicSlice + // Slice V10 & DynamicSlice void FillVectorsFromInput(const OpKernelContext* context, std::vector& input_starts, std::vector& input_ends, diff --git a/onnxruntime/core/providers/cpu/tensor/utils.h b/onnxruntime/core/providers/cpu/tensor/utils.h index 0eb46d8ce9..3f24286eb8 100644 --- a/onnxruntime/core/providers/cpu/tensor/utils.h +++ b/onnxruntime/core/providers/cpu/tensor/utils.h @@ -120,15 +120,15 @@ struct ExtentAxisCounters { }; // A std::vector that holds the number of entries to skip to go to the next axis start given an extent -// and optionally steps along each axis: +// and optionally steps along each axis: // This is used by the SliceIterator to iterate over a slice of a tensor struct SliceSkips : std::vector { SliceSkips(const TensorShape& input_shape, gsl::span extents, gsl::span steps) - : std::vector(input_shape.NumDimensions(), 0) { + : std::vector(input_shape.NumDimensions(), 0) { auto& dims = input_shape.GetDims(); ORT_ENFORCE(static_cast(dims.size()) == extents.size() && static_cast(dims.size()) >= steps.size()); - + int64_t inner_most_dim = dims.size() - 1; // assume step == 1 if not present ptrdiff_t steps_size = steps.size(); @@ -140,48 +140,47 @@ struct SliceSkips : std::vector { for (size_t i = size(); i-- > 0;) { auto prevPitch = pitch; pitch *= dims[i]; - + // assume step == 1 if not present int64_t steps_i_minus_1 = 1; if (i > 0 && static_cast(i) - 1 < steps_size) - steps_i_minus_1 = steps[i - 1]; + steps_i_minus_1 = steps[i - 1]; // first "revert" back to the old starting position (term with -ve sign) // and then "step" over the pitch accordingly (term with +ve sign) operator[](i) = steps_i_minus_1 * pitch - steps_i * extents[i] * prevPitch; - + steps_i = steps_i_minus_1; } } }; -// This provides easy sequential iteration over a subset of a tensor given a span of starts, extents & optionally steps +// This provides easy sequential iteration over a subset of a tensor given a span of starts, extents & optionally steps template struct SliceIterator { - SliceIterator(const Tensor& tensor, gsl::span starts, + SliceIterator(const Tensor& tensor, gsl::span starts, gsl::span extents, gsl::span steps) - : tensor_(tensor), extents_(extents), skips_(tensor_.Shape(), extents, steps), indices_(extents.size(), 0) { + : tensor_(tensor), extents_(extents), skips_(tensor_.Shape(), extents, steps), indices_(extents.size(), 0) { auto& dims = tensor_.Shape().GetDims(); Init(dims, starts, steps); } - + // This construct takes a explicit tensor_shape which might be different from the shape defined in input tensor. // The explicit tensor_shape usually has inner most axis flattened. For example, given shape[1,4,4,2], if last axis // does not have padding or slice, then it will be flattened as [1,4,8] for better performance (One inner most copy instead of 4). // Also supports arbitrary positive and negative stepping along individual axes - SliceIterator(const Tensor& tensor, const TensorShape& tensor_shape, gsl::span starts, + SliceIterator(const Tensor& tensor, const TensorShape& tensor_shape, gsl::span starts, gsl::span extents, gsl::span steps) - : tensor_(tensor), extents_(extents), skips_(tensor_shape, extents, steps), indices_(extents.size(), 0) { + : tensor_(tensor), extents_(extents), skips_(tensor_shape, extents, steps), indices_(extents.size(), 0) { auto& dims = tensor_shape.GetDims(); Init(dims, starts, steps); } // Initialize initial skip and inner_extent. - void Init(const std::vector& dims, gsl::span starts, + void Init(const std::vector& dims, gsl::span starts, gsl::span steps) { - - ORT_ENFORCE(static_cast(dims.size()) == starts.size() && - static_cast(dims.size()) == extents_.size() && + ORT_ENFORCE(static_cast(dims.size()) == starts.size() && + static_cast(dims.size()) == extents_.size() && static_cast(dims.size()) >= steps.size()); size_t pitch = 1; @@ -192,8 +191,9 @@ struct SliceIterator { } inner_extent_ = extents_[dims.size() - 1]; - inner_step_ = static_cast(dims.size()) == steps.size() - ? steps[dims.size() - 1] : 1; + inner_step_ = static_cast(dims.size()) == steps.size() + ? steps[dims.size() - 1] + : 1; } void AdvanceOverInnerExtent() { @@ -205,18 +205,33 @@ struct SliceIterator { } } - const T* operator++(int) { - const T* input = input_; + void IncrementInnerDimension() { input_ += inner_step_; if (++inner_counter_ == inner_extent_) { inner_counter_ = 0; AdvanceOverInnerExtent(); } + } + + // postfix iterator increment + const T* operator++(int) { + const T* input = input_; + IncrementInnerDimension(); return input; } - // spliting the function that copies the innermost dimension into 2 separate methods, - // as this is most likely being called within a loop + // prefix iterator increment + const T* operator++() { + IncrementInnerDimension(); + return input_; + } + + const T& operator*() const { + return *input_; + } + + // spliting the function that copies the innermost dimension into 2 separate methods, + // as this is most likely being called within a loop // and we want to avoid the check inside to avoid overhead // upto the caller to call the relevant one